Re: Add contrib/pg_logicalsnapinspect

On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot
<bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
>
> Hi,
>
> On Tue, Oct 08, 2024 at 10:52:11AM -0700, Masahiko Sawada wrote:
> > On Tue, Oct 8, 2024 at 9:25 AM Bertrand Drouvot
> > <bertranddrouvot(dot)pg(at)gmail(dot)com> wrote:
> >
> > Thank you for updating the patch! I have some comments on v12 patch:
>
> Thanks for looking at it!
>
> > ---
> > + if (ondisk.builder.committed.xcnt > 0)
> > + {
> > + Datum *arrayelems;
> > + int narrayelems = 0;
> > +
> > + arrayelems = (Datum *)
> > palloc(ondisk.builder.committed.xcnt * sizeof(Datum));
> > +
> > + for (; narrayelems < ondisk.builder.committed.xcnt;
> > narrayelems++)
> > + arrayelems[narrayelems] =
> > Int64GetDatum((int64) ondisk.builder.committed.xip[narrayelems]);
> > +
> > + values[i++] =
> > PointerGetDatum(construct_array_builtin(arrayelems, narrayelems,
> > INT8OID));
> > + }
> >
> > Since committed_xip and catchange_xip are xid[], we should use
> > TransactionIdGetDatum() and XIDOID instead.
>
> I ended up using INT8OID because XIDOID is not part of the switch in
> construct_array_builtin() and so leads to:
>
> "
> ERROR: type 28 not supported by construct_array_builtin()
> "

Thank you for pointing it out.

>
> One option could be (did not test it) to add this switch in construct_array_builtin():
>
> + case XIDOID:
> + elmlen = sizeof(TransactionId);
> + elmbyval = true;
> + elmalign = TYPALIGN_INT;
> + break;
>
> I think that could make sense and would probably need a dedicated patch for that,
> thoughts?

Or can we use construct_array() instead?

> > ---
> > +# Test the pg_logicalinspect functions: that needs some permutation to
> > +# ensure that we are creating multiple logical snapshots and that one of them
> > +# contains ongoing catalogs changes.
> >
> > If we use prepared transactions modifying catalog changes, can we
> > write the normal (i.e. not isolation check) tests? It would be easier
> > to write and add tests.
> >
>
> Not sure about this one. I think that the test is simple enough and mainly inspired
> by what can be found in the test_decoding module.
>
> We could still add "normal" (REGRESS) tests in the future should we add features
> to the pg_logicalinspect module that would require new tests.
>
> For example, test_decoding is using both kind of tests, what do you think?

Fair point. I agree with you.

Regards,

--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com