Re: Code cleanup for detoast a expanded datum.

Hi,

When detoasting an expanded datum in detoast_attr, we already know it is
an expanded datum, but we use deotast_external_attr function which checks
VARATT_IS_EXTERNAL_ONDISK & VARATT_IS_EXTERNAL_INDIRECT again which I
think it is impossible to true in this case. so I think we should use a
more specific code to handle this, see the attached patch.

We already have lots of kind of toast type and some of checks have
overlap, this cleanup should make people less confused when reading this
code.

make check-world passed after applying this patch.

Any thought?

--
Best Regards
Andy Fan

Andy Fan <zhihuifan1213(at)163(dot)com> writes:

>
> make check-world passed after applying this patch.

v2 changes the places of Assert, which is missed in v1 by mistakes.

--
Best Regards
Andy Fan

On Mon, Nov 18, 2024 at 7:42 PM Andy Fan <zhihuifan1213(at)163(dot)com> wrote:

> Andy Fan <zhihuifan1213(at)163(dot)com> writes:
>
> >
> > make check-world passed after applying this patch.
>
> v2 changes the places of Assert, which is missed in v1 by mistakes.
>

I'm not an expert in this end of the code but it looks correct to me, my
only comment would be maybe add a new function
detoast_external_expanded_attr that is called from both
detoast_external_attr and detoast_attr so the EOHP stuff stays hidden
behind a function.

If that's too much API commitment then I'm fine with the way it looks
inlining the EOHP code as you have to avoid the duplicate checks.

-Michel

Michel Pelletier <pelletier(dot)michel(at)gmail(dot)com> writes:

> On Mon, Nov 18, 2024 at 7:42 PM Andy Fan <zhihuifan1213(at)163(dot)com> wrote:
>
> Andy Fan <zhihuifan1213(at)163(dot)com> writes:
>
> >
> > make check-world passed after applying this patch.
>
> v2 changes the places of Assert, which is missed in v1 by mistakes.
>
> I'm not an expert in this end of the code but it looks correct to me,
> my only comment would be maybe add a new function
> detoast_external_expanded_attr that is called from both
> detoast_external_attr and detoast_attr so the EOHP stuff stays
> hidden behind a function.

Thanks for the double check. Your suggestion looks good to me. v3 is
attached for this.

Just that I'm not sure about the function name. To be
consistent with other functions in this area, e.g. toast_fetch_datum,
totast_decompressed_datum, it looks like this function would be
toast_EOH/expaned_attr. Currently I'm using toast_EOH_attr. I'm open for
other names.

I find detoast_attr_slice has the similar issues as detoast_attr, so
included in v3 as well.

--
Best Regards
Andy Fan