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

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()
"

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?

> I think that it would be cleaner if we pass
> ondisk.builder.committed.xcnt instead of construct_array_builtin to
> construct_array_buildin(). That is, we can rewrite it as follows:
> 
> for (int j = 0; j < ondisk.builder.committed.xcnt; j++)
>     arrayelems[j] = TransactionIdGetDatum(ondisk.builder.committed.xip[j]);
> 
> values[i++] = PointerGetDatum(construct_array_builtin(arrayelems,
> ondisk.builder.committed.xcnt, XIDOID));

Fine by me. Will do that in v13 with TransactionIdGetDatum/XIDOID or Int64GetDatum/INT8OID
once we decide what to do with the above remark linked to construct_array_builtin().

> ---
> +# 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? 

> ---
> +       tuple = heap_form_tuple(tupdesc, values, nulls);
> +
> +       MemoryContextReset(context);
> +
> +       PG_RETURN_DATUM(HeapTupleGetDatum(tuple));
> 
> I think we don't necessarily need to reset the memory context here.
> Rather, I think we can just pass CurrentMemoryContext to
> ValidateAndRestoreSnapshotFile() instead of passing the separate new
> memory context.

Yeah, we should be in a short-lived memory context here (ExprContext or such),
so that's fine by me (will do in v13).

> ---
> +       fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
> +
> +       if (fd < 0)
> +               ereport(ERROR,
> +                               (errcode_for_file_access(),
> +                                errmsg("could not open file \"%s\":
> %m", path)));
> +
> +       context = AllocSetContextCreate(CurrentMemoryContext,
> +
>  "logicalsnapshot inspect context",
> +
>  ALLOCSET_DEFAULT_SIZES);
> +
> +       /* Validate and restore the snapshot to 'ondisk' */
> +       ValidateAndRestoreSnapshotFile(&ondisk, path, fd, context);
> 
> It's a bit odd to me that this function opens a snapshot file and
> passes both the file descriptor and file path. The file path is used
> mostly only for error reporting in ValidateAndRestoreSnapshotFile().

Right.

> I guess it would be cleaner if we pass the file path to
> ValidateAndRestoreSnapshotFile() which opens and validates the
> snapshot file. Since SnapBuildRestore() wants to get false if the
> specified file doesn't exist, we can also add missing_ok argument to
> ValidateAndRestoreSnapshotFile(). That is, the function will be like:
> 
> void
> ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char
> *path, MemoryContext context, bool missing_ok)
> {
> :
>     fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
> 
>     if (fd < 0)
>     {
>         if (missing_ok && errno == ENOENT)
>             return false;
>         else
>             ereport(ERROR,
>                 (errcode_for_file_access(),
>                  errmsg("could not open file \"%s\": %m", path)));
>     }

Yeah, it makes sense to move the OpenTransientFile() call in
ValidateAndRestoreSnapshotFile(), will do in v13.

Regards,

-- 
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com