On Mon, May 15, 2023 at 11:00:00AM +0300, Alexander Lakhin wrote:
> Yes, the existing implementation might crash with non-leaf pages and just
> doesn't show non-key columns for items on leaves.
Ah, OK. After more caffeine.. So that's two problems, not one.
>> The test output is a bit bloated. Could it be possible to make it
>> shorter, like limiting the output to the first and last items, for
>> example, as the goal is mainly to check the output format? This could
>> check for the number of tuples, as well, but I am wondering if this
>> would be stable across the buildfarm.
>
> Yeah, the test output could be shorter and more representative at the same
> time. Please look at the improved test.
Sounds fine as it is, thanks for the update. The patch is now much
shorter in size.
> It sounds good to me, but for now I can see only a single caller of
> pg_get_indexdef_columns(), so maybe it would be harmless to merely change the
> existing function's parameter on HEAD? (To not have two distinct functions
> with generic parameters for just two callers, which will use only two
> fixed parameter values.)
Sure, we could do that for HEAD, but we would still have to maintain
an extra routine for the back-branches. So I would tend to choose an
approach that's consistent across all the branches while being as much
as possible extensible.
Something similar could be said about the lack of extensibility of
BuildIndexValueDescription(). However, note also its top comment
telling about the fact that the function is used only for the
generation of error messages, which is something that pageinspect has
actually violated since we have the item functions for GiST :)
As you said upthread, there may be an argument in making an extra
version of BuildIndexValueDescription() to be aware of included
attributes but I cannot get much excited about adding an extra code
path in genam.c without something else than just begin able to look at
the GiST items on a page. And BuildIndexValueDescription() has a few
ACL checks while pageinspect requires superuser rights, so that makes
the module a bit faster, so agreed with your points.
+ tupdesc = CreateTupleDescCopy(RelationGetDescr(indexRel));
+ tupdesc->natts = IndexRelationGetNumberOfKeyAttributes(indexRel);
Ah, I see. So you need that to be able to deform correctly the tuple
coming from a non-leaf page that only has key attributes. Tricky at
first sight, indeed..
I have done a few adjustments to that patch, refining some comments,
fixing some ordering with the headers and applying an indentation. Is
that OK for you?
--
Michael
