On Fri, May 12, 2023 at 01:00:01PM +0300, Alexander Lakhin wrote:
> Please look at the patch attached, that makes gist_page_items() process items
> on leaf and non-leaf pages differently, accounting for their contents.
> I've decided to move away from BuildIndexValueDescription(), but borrow some
> of it's code, for two reasons:
> that function outputs only key columns;
> it checks permissions for a table/index, but this is not needed for pageinspect
> (firstly, because a user already has all the data do decode;
> secondly, because gist_page_items() requires a superuser role anyway).
I have not looked in details at the patch, and GiST is going to
require some analysis, still I have a few comments after a lookup :)
Anyway, is it right to assume that gist_page_items() is basically
incorrect now with an included index, especially if one scans a range
of pages, hitting leaves?
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.
+ if (itup_isnull[i])
+ val = "null";
The patch includes something for a NULL value, but does not seem to
output anything related to it in the regression test.
+extern char *pg_get_indexdef_columns_with_included(Oid indexrelid,
+ bool pretty);
Hmm. For back-branches, this approach would be fine, and even on HEAD
I would not remove pg_get_indexdef_columns(). Still, for this new
one, would it be better to have an _extended() version of this routine
with some bits32 to control the addition of included columns and the
pretty flag? For the API published, only these two would be
necessary.
+ (1,"(0,1)",48,f,"(p) INCLUDE (t)=((1,1)) INCLUDE (1)")
Agreed that this format kind of makes sense, showing both the
attribute included and the value include.
--
Michael
