Date: Wed, 9 Oct 2024 08:12:32 +0000
From: Bertrand Drouvot <bertranddrouvot.pg@gmail.com>
To: Masahiko Sawada <sawada.mshk@gmail.com>
Cc: Peter Smith <smithpb2250@gmail.com>,
	Peter Eisentraut <peter@eisentraut.org>,
	shveta malik <shveta.malik@gmail.com>,
	Amit Kapila <amit.kapila16@gmail.com>,
	Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com>,
	pgsql-hackers@lists.postgresql.org
Subject: Re: Add contrib/pg_logicalsnapinspect
Message-ID: <ZwY68OhKx/5jWeCj@ip-10-97-1-34.eu-west-3.compute.internal>
References: <Zu0bod6+zj0v+7wd@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAHut+PsVj9+DO6_zQrW8-n47DxuxFzwHKXOJi6AE3M9Yki9XPA@mail.gmail.com>
 <ZvLuDfEUow6gw2Hr@ip-10-97-1-34.eu-west-3.compute.internal>
 <6dd0f118-ce86-4b98-af2c-1319e1593a15@eisentraut.org>
 <ZvRIZctV1M4PPmwT@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoBHkL-C22pgGJCA_8Jv6HjYq0c_1Y_b2Xktk7_SdUqxqA@mail.gmail.com>
 <ZwOkPDCKQ/gDNuZL@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAHut+PvtLb6g1L9C_zbufuHbJTWNq-BeseCpVgjsabG0md6_8g@mail.gmail.com>
 <ZwVc8SZWvTlzbE6x@ip-10-97-1-34.eu-west-3.compute.internal>
 <CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
Content-Transfer-Encoding: 8bit
In-Reply-To: <CAD21AoAO3zyooPQ+E2rpEi1YBc+gxZr0CUtJwxaFJ-WWimJNvA@mail.gmail.com>
Archived-At: <https://www.postgresql.org/message-id/ZwY68OhKx/5jWeCj%40ip-10-97-1-34.eu-west-3.compute.internal>
Precedence: bulk

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