Re: Add contrib/pg_logicalsnapinspect - Mailing list pgsql-hackers

On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.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.pg@gmail.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