Re: Add contrib/pg_logicalsnapinspect

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

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