Thread: Re: Add contrib/pg_logicalsnapinspect
On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Please find attached a patch to $SUBJECT. > > This module provides SQL functions to inspect the contents of serialized logical > snapshots of a running database cluster, which I think could be useful for > debugging or educational purposes. > +1. I see it could be a good debugging aid. > > 2. The SnapBuildOnDisk and SnapBuild structs are now exposed to public. Means > we should now pay much more attention when changing their contents but I think > it's worth it. > Is it possible to avoid exposing these structures? Can we expose some function from snapbuild.c that provides the required information? -- With Regards, Amit Kapila.
Hi, On Mon, Aug 26, 2024 at 07:05:27PM +0530, Amit Kapila wrote: > On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Please find attached a patch to $SUBJECT. > > > > This module provides SQL functions to inspect the contents of serialized logical > > snapshots of a running database cluster, which I think could be useful for > > debugging or educational purposes. > > > > +1. I see it could be a good debugging aid. Thanks for the feedback! > > > > 2. The SnapBuildOnDisk and SnapBuild structs are now exposed to public. Means > > we should now pay much more attention when changing their contents but I think > > it's worth it. > > > > Is it possible to avoid exposing these structures? Can we expose some > function from snapbuild.c that provides the required information? Yeah, that's an option if we don't want to expose those structs to public. I think we could 1/ create a function that would return a formed HeapTuple, or 2/ we could create multiple functions (about 15) that would return the values we are interested in. I think 2/ is fine as it would give more flexiblity (no need to retrieve a whole tuple if one is interested to only one value). What do you think? Did you have something else in mind? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Wed, Aug 28, 2024 at 1:25 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > On Mon, Aug 26, 2024 at 07:05:27PM +0530, Amit Kapila wrote: > > On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > 2. The SnapBuildOnDisk and SnapBuild structs are now exposed to public. Means > > > we should now pay much more attention when changing their contents but I think > > > it's worth it. > > > > > > > Is it possible to avoid exposing these structures? Can we expose some > > function from snapbuild.c that provides the required information? > > Yeah, that's an option if we don't want to expose those structs to public. > > I think we could 1/ create a function that would return a formed HeapTuple, or > 2/ we could create multiple functions (about 15) that would return the values > we are interested in. > > I think 2/ is fine as it would give more flexiblity (no need to retrieve a whole > tuple if one is interested to only one value). > True, but OTOH, each time we add a new field to these structures, a new function has to be exposed. I don't have a strong opinion on this but seeing current use cases, it seems okay to expose a single function. > What do you think? Did you have something else in mind? > On similar lines, we can also provide a function to get the slot's on-disk data. IIRC, Bharath had previously proposed a tool to achieve the same. It is fine if we don't want to add that as part of this patch but I mentioned it because by having that we can have a set of functions to view logical decoding data. -- With Regards, Amit Kapila.
Hi, On Thu, Aug 29, 2024 at 02:51:36PM +0530, Amit Kapila wrote: > On Wed, Aug 28, 2024 at 1:25 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > On Mon, Aug 26, 2024 at 07:05:27PM +0530, Amit Kapila wrote: > > > On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot > > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > > > 2. The SnapBuildOnDisk and SnapBuild structs are now exposed to public. Means > > > > we should now pay much more attention when changing their contents but I think > > > > it's worth it. > > > > > > > > > > Is it possible to avoid exposing these structures? Can we expose some > > > function from snapbuild.c that provides the required information? > > > > Yeah, that's an option if we don't want to expose those structs to public. > > > > I think we could 1/ create a function that would return a formed HeapTuple, or > > 2/ we could create multiple functions (about 15) that would return the values > > we are interested in. > > > > I think 2/ is fine as it would give more flexiblity (no need to retrieve a whole > > tuple if one is interested to only one value). > > > > True, but OTOH, each time we add a new field to these structures, a > new function has to be exposed. I don't have a strong opinion on this > but seeing current use cases, it seems okay to expose a single > function. Yeah that's fair. And now I'm wondering if we need an extra module. I think we could "simply" expose 2 new functions in core, thoughts? > > What do you think? Did you have something else in mind? > > > > On similar lines, we can also provide a function to get the slot's > on-disk data. Yeah, having a way to expose the data from the disk makes fully sense to me. > IIRC, Bharath had previously proposed a tool to achieve > the same. It is fine if we don't want to add that as part of this > patch but I mentioned it because by having that we can have a set of > functions to view logical decoding data. > That's right. I think this one would be simply enough to expose one or two functions in core too (and probably would not need an extra module). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Yeah that's fair. And now I'm wondering if we need an extra module. I think > we could "simply" expose 2 new functions in core, thoughts? > > > > What do you think? Did you have something else in mind? > > > > > > > On similar lines, we can also provide a function to get the slot's > > on-disk data. > > Yeah, having a way to expose the data from the disk makes fully sense to me. > > > IIRC, Bharath had previously proposed a tool to achieve > > the same. It is fine if we don't want to add that as part of this > > patch but I mentioned it because by having that we can have a set of > > functions to view logical decoding data. > > That's right. I think this one would be simply enough to expose one or two > functions in core too (and probably would not need an extra module). +1 for functions in core unless this extra module pg_logicalsnapinspect works as a tool to be helpful even when the server is down. FWIW, I wrote pg_replslotdata as a tool, not as an extension for reading on-disk replication slot data to help when the server is down - https://www.postgresql.org/message-id/flat/CALj2ACW0rV5gWK8A3m6_X62qH%2BVfaq5hznC%3Di0R5Wojt5%2Byhyw%40mail.gmail.com. When the server is running, pg_get_replication_slots() pretty much gives the on-disk contents. -- Bharath Rupireddy PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi, On Thu, Aug 29, 2024 at 06:33:19PM +0530, Bharath Rupireddy wrote: > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > That's right. I think this one would be simply enough to expose one or two > > functions in core too (and probably would not need an extra module). > > +1 for functions in core unless this extra module > pg_logicalsnapinspect works as a tool to be helpful even when the > server is down. Thanks for the feedback! I don't see any use case where it could be useful when the server is down. So, I think I'll move forward with in core functions (unless someone has a different opinion). Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> wrote: > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > Yeah that's fair. And now I'm wondering if we need an extra module. I think > > we could "simply" expose 2 new functions in core, thoughts? > > > > > > What do you think? Did you have something else in mind? > > > > > > > > > > On similar lines, we can also provide a function to get the slot's > > > on-disk data. > > > > Yeah, having a way to expose the data from the disk makes fully sense to me. > > > > > IIRC, Bharath had previously proposed a tool to achieve > > > the same. It is fine if we don't want to add that as part of this > > > patch but I mentioned it because by having that we can have a set of > > > functions to view logical decoding data. > > > > That's right. I think this one would be simply enough to expose one or two > > functions in core too (and probably would not need an extra module). > > +1 for functions in core unless this extra module > pg_logicalsnapinspect works as a tool to be helpful even when the > server is down. > We have an example of pageinspect which provides low-level functions to aid debugging. The proposal for these APIs also seems to fall in the same category, so why go for the core for these functions? -- With Regards, Amit Kapila.
Hi, On Fri, Aug 30, 2024 at 03:43:12PM +0530, Amit Kapila wrote: > On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > Yeah that's fair. And now I'm wondering if we need an extra module. I think > > > we could "simply" expose 2 new functions in core, thoughts? > > > > > > > > What do you think? Did you have something else in mind? > > > > > > > > > > > > > On similar lines, we can also provide a function to get the slot's > > > > on-disk data. > > > > > > Yeah, having a way to expose the data from the disk makes fully sense to me. > > > > > > > IIRC, Bharath had previously proposed a tool to achieve > > > > the same. It is fine if we don't want to add that as part of this > > > > patch but I mentioned it because by having that we can have a set of > > > > functions to view logical decoding data. > > > > > > That's right. I think this one would be simply enough to expose one or two > > > functions in core too (and probably would not need an extra module). > > > > +1 for functions in core unless this extra module > > pg_logicalsnapinspect works as a tool to be helpful even when the > > server is down. > > > > We have an example of pageinspect which provides low-level functions > to aid debugging. The proposal for these APIs also seems to fall in > the same category, That's right, but... > so why go for the core for these functions? as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to public and to create/expose 2 new functions in snapbuild.c then the functions in the module would do nothing but expose the data coming from the snapbuild.c's functions (get the tuple and send it to the client). That sounds weird to me to create a module that would "only" do so, that's why I thought that in core functions taking care of everything make more sense. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > On Fri, Aug 30, 2024 at 03:43:12PM +0530, Amit Kapila wrote: > > On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy > > <bharath.rupireddyforpostgres@gmail.com> wrote: > > > > > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot > > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > > > Yeah that's fair. And now I'm wondering if we need an extra module. I think > > > > we could "simply" expose 2 new functions in core, thoughts? > > > > > > > > > > What do you think? Did you have something else in mind? > > > > > > > > > > > > > > > > On similar lines, we can also provide a function to get the slot's > > > > > on-disk data. > > > > > > > > Yeah, having a way to expose the data from the disk makes fully sense to me. > > > > > > > > > IIRC, Bharath had previously proposed a tool to achieve > > > > > the same. It is fine if we don't want to add that as part of this > > > > > patch but I mentioned it because by having that we can have a set of > > > > > functions to view logical decoding data. > > > > > > > > That's right. I think this one would be simply enough to expose one or two > > > > functions in core too (and probably would not need an extra module). > > > > > > +1 for functions in core unless this extra module > > > pg_logicalsnapinspect works as a tool to be helpful even when the > > > server is down. > > > > > > > We have an example of pageinspect which provides low-level functions > > to aid debugging. The proposal for these APIs also seems to fall in > > the same category, > > That's right, but... > > > so why go for the core for these functions? > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to public > and to create/expose 2 new functions in snapbuild.c then the functions in the > module would do nothing but expose the data coming from the snapbuild.c's > functions (get the tuple and send it to the client). That sounds weird to me to > create a module that would "only" do so, that's why I thought that in core > functions taking care of everything make more sense. > I see your point. Does anyone else have an opinion on the need for these functions and whether to expose them from a contrib module or have them as core functions? -- With Regards, Amit Kapila.
Hi, On Mon, Sep 09, 2024 at 04:24:09PM +0530, Amit Kapila wrote: > On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to public > > and to create/expose 2 new functions in snapbuild.c then the functions in the > > module would do nothing but expose the data coming from the snapbuild.c's > > functions (get the tuple and send it to the client). That sounds weird to me to > > create a module that would "only" do so, that's why I thought that in core > > functions taking care of everything make more sense. > > > > I see your point. Does anyone else have an opinion on the need for > these functions and whether to expose them from a contrib module or > have them as core functions? I looked at when the SNAPBUILD_VERSION has been changed: ec5896aed3 (2014) a975ff4980 (2021) 8bdb1332eb (2021) 7f13ac8123 (2022) bb19b70081 (2024) So it's not like we are changing the SnapBuildOnDisk or SnapBuild structs that frequently. Furthermore, those structs are serialized and so we have to preserve their on-disk compatibility (means we can change them only in a major release if we need to). So, I think it would not be that much of an issue to expose those structs and create a new contrib module (as v1 did propose) instead of in core new functions. If we want to insist that external modules "should" not rely on those structs then we could put them into a new internal_snapbuild.h file (instead of snapbuild.h as proposed in v1). At the end, I think that creating a contrib module and exposing those structs in internal_snapbuild.h make more sense (as compared to in core functions). Thoughts? Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Tue, Sep 10, 2024 at 8:56 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > On Mon, Sep 09, 2024 at 04:24:09PM +0530, Amit Kapila wrote: > > On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to public > > > and to create/expose 2 new functions in snapbuild.c then the functions in the > > > module would do nothing but expose the data coming from the snapbuild.c's > > > functions (get the tuple and send it to the client). That sounds weird to me to > > > create a module that would "only" do so, that's why I thought that in core > > > functions taking care of everything make more sense. > > > > > > > I see your point. Does anyone else have an opinion on the need for > > these functions and whether to expose them from a contrib module or > > have them as core functions? > > I looked at when the SNAPBUILD_VERSION has been changed: > > ec5896aed3 (2014) > a975ff4980 (2021) > 8bdb1332eb (2021) > 7f13ac8123 (2022) > bb19b70081 (2024) > > So it's not like we are changing the SnapBuildOnDisk or SnapBuild structs that > frequently. Furthermore, those structs are serialized and so we have to preserve > their on-disk compatibility (means we can change them only in a major release > if we need to). > > So, I think it would not be that much of an issue to expose those structs and > create a new contrib module (as v1 did propose) instead of in core new functions. > > If we want to insist that external modules "should" not rely on those structs then > we could put them into a new internal_snapbuild.h file (instead of snapbuild.h > as proposed in v1). > Adding snapbuild_internal.h sounds like a good idea. > At the end, I think that creating a contrib module and exposing those structs in > internal_snapbuild.h make more sense (as compared to in core functions). > Fail enough. We can keep the module name as logicalinspect so that we can extend it for other logical decoding/replication-related files in the future. -- With Regards, Amit Kapila.
On Wed, Sep 11, 2024 at 4:21 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
>
> Yeah, good idea. Done that way in v3 attached.
>
Thanks for the patch. +1 on the patch's idea. I have started
reviewing/testing it. It is WIP but please find few initial comments:
src/backend/replication/logical/snapbuild.c:
1)
+ fsync_fname("pg_logical/snapshots", true);
Should we use macros PG_LOGICAL_DIR and PG_LOGICAL_SNAPSHOTS_DIR in
ValidateSnapshotFile(), instead of hard coding the path
2)
Same as above in pg_get_logical_snapshot_meta() and
pg_get_logical_snapshot_info()
+ sprintf(path, "pg_logical/snapshots/%X-%X.snap",
+ LSN_FORMAT_ARGS(lsn)); LSN_FORMAT_ARGS(lsn));
3)
+#include "replication/internal_snapbuild.h"
Shall we name new file as 'snapbuild_internal.h' instead of
'internal_snapbuild.h'. Please see other files' name under
'./src/include/replication':
worker_internal.h
walsender_private.h
4)
+static void ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
+ const char *path);
Is it required? We generally don't add declaration unless required by
compiler. Since definition is prior to usage, it is not needed?
thanks
Shveta
On Mon, Sep 16, 2024 at 8:03 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Sep 16, 2024 at 04:02:51PM +0530, shveta malik wrote:
> > On Wed, Sep 11, 2024 at 4:21 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > >
> > > Yeah, good idea. Done that way in v3 attached.
> > >
> >
> > Thanks for the patch. +1 on the patch's idea. I have started
> > reviewing/testing it. It is WIP but please find few initial comments:
>
> Thanks for sharing your thoughts and for the review!
>
> >
> > src/backend/replication/logical/snapbuild.c:
> >
> > 1)
> > + fsync_fname("pg_logical/snapshots", true);
> >
> > Should we use macros PG_LOGICAL_DIR and PG_LOGICAL_SNAPSHOTS_DIR in
> > ValidateSnapshotFile(), instead of hard coding the path
> >
> >
> > 2)
> > Same as above in pg_get_logical_snapshot_meta() and
> > pg_get_logical_snapshot_info()
> >
> > + sprintf(path, "pg_logical/snapshots/%X-%X.snap",
> > + LSN_FORMAT_ARGS(lsn)); LSN_FORMAT_ARGS(lsn));
> >
>
> Doh! Yeah, agree that we should use those macros. They are coming from c39afc38cf
> which has been introduced after v1 of this patch. I thought I took care of it once
> c39afc38cf went in, but it looks like I missed it somehow. Done in v4 attached,
> Thanks!
>
> > 3)
> > +#include "replication/internal_snapbuild.h"
> >
> > Shall we name new file as 'snapbuild_internal.h' instead of
> > 'internal_snapbuild.h'. Please see other files' name under
> > './src/include/replication':
> > worker_internal.h
> > walsender_private.h
>
> Agree, that should be snapbuild_internal.h, done in v4.
>
> >
> > 4)
> > +static void ValidateSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
> > + const char *path);
> >
> > Is it required? We generally don't add declaration unless required by
> > compiler. Since definition is prior to usage, it is not needed?
> >
>
> I personally prefer to add them even if not required by the compiler. I did not
> pay attention that "We generally don't add declaration unless required by compiler"
> and (after a quick check) I did not find any reference in the coding style
> documentation [1]. That said, I don't have a strong opinion about that and so
> removed in v4. Worth to add a mention in the coding convention doc?
>
Okay. I was somehow under the impression that this is the way in the
postgres i.e. not add redundant declarations. Will be good to know
what others think on this.
Thanks for addressing the comments. I have not started reviewing v4
yet, but here are few more comments on v3:
1)
+#include "port/pg_crc32c.h"
It is not needed in pg_logicalinspect.c as it is already included in
internal_snapbuild.h
2)
+ values[0] = Int16GetDatum(ondisk.builder.state);
........
+ values[8] = LSNGetDatum(ondisk.builder.last_serialized_snapshot);
+ values[9] = TransactionIdGetDatum(ondisk.builder.next_phase_at);
+ values[10] = Int64GetDatum(ondisk.builder.committed.xcnt);
We can have values[i++] in all the places and later we can check :
Assert(i == PG_GET_LOGICAL_SNAPSHOT_INFO_COLS);
Then we need not to keep track of number even in later part of code,
as it goes till 14.
3)
Similar change can be done here:
+ values[0] = Int32GetDatum(ondisk.magic);
+ values[1] = Int32GetDatum(ondisk.checksum);
+ values[2] = Int32GetDatum(ondisk.version);
check at the end will be: Assert(i == PG_GET_LOGICAL_SNAPSHOT_META_COLS);
4)
Most of the output columns in pg_get_logical_snapshot_info() look
self-explanatory except 'state'. Should we have meaningful 'text' here
corresponding to SnapBuildState? Similar to what we do for
'invalidation_reason' in pg_replication_slots. (SlotInvalidationCauses
for ReplicationSlotInvalidationCause)
thanks
Shveta
On Tue, Sep 17, 2024 at 10:18 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> Thanks for addressing the comments. I have not started reviewing v4
> yet, but here are few more comments on v3:
>
I just noticed that when we pass NULL input, both the new functions
give 1 row as output, all cols as NULL:
newdb1=# SELECT * FROM pg_get_logical_snapshot_meta(NULL);
magic | checksum | version
-------+----------+---------
| |
(1 row)
Similar behavior with pg_get_logical_snapshot_info(). While the
existing 'pg_ls_logicalsnapdir' function gives this error, which looks
more meaningful:
newdb1=# select * from pg_ls_logicalsnapdir(NULL);
ERROR: function pg_ls_logicalsnapdir(unknown) does not exist
LINE 1: select * from pg_ls_logicalsnapdir(NULL);
HINT: No function matches the given name and argument types. You
might need to add explicit type casts.
Shouldn't the new functions have same behavior?
thanks
Shveta
On Monday, September 16, 2024, shveta malik <shveta.malik@gmail.com> wrote:
On Tue, Sep 17, 2024 at 10:18 AM shveta malik <shveta.malik@gmail.com> wrote:
>
> Thanks for addressing the comments. I have not started reviewing v4
> yet, but here are few more comments on v3:
>
I just noticed that when we pass NULL input, both the new functions
give 1 row as output, all cols as NULL:
newdb1=# SELECT * FROM pg_get_logical_snapshot_meta(NULL);
magic | checksum | version
-------+----------+---------
| |
(1 row)
Similar behavior with pg_get_logical_snapshot_info(). While the
existing 'pg_ls_logicalsnapdir' function gives this error, which looks
more meaningful:
newdb1=# select * from pg_ls_logicalsnapdir(NULL);
ERROR: function pg_ls_logicalsnapdir(unknown) does not exist
LINE 1: select * from pg_ls_logicalsnapdir(NULL);
HINT: No function matches the given name and argument types. You
might need to add explicit type casts.
Shouldn't the new functions have same behavior?
No. Since the name pg_ls_logicalsnapdir has zero single-argument implementations passing a null value as an argument is indeed attempt to invoke a function signature that doesn’t exist.
If there is exactly one single input argument function of the given name the parser is going to cast the null literal to the data type of the single argument and invoke the function. It will not and cannot be convinced to fail to find a matching function.
I can see an argument that they should produce an empty set instead of a single all-null row, but the idea that they wouldn’t even be found is contrary to a core design of the system.
David J.
Hi,
On Mon, Sep 16, 2024 at 10:16:16PM -0700, David G. Johnston wrote:
> On Monday, September 16, 2024, shveta malik <shveta.malik@gmail.com> wrote:
>
> > On Tue, Sep 17, 2024 at 10:18 AM shveta malik <shveta.malik@gmail.com>
> > wrote:
> > >
> > > Thanks for addressing the comments. I have not started reviewing v4
> > > yet, but here are few more comments on v3:
> > >
> >
> > I just noticed that when we pass NULL input, both the new functions
> > give 1 row as output, all cols as NULL:
> >
> > newdb1=# SELECT * FROM pg_get_logical_snapshot_meta(NULL);
> > magic | checksum | version
> > -------+----------+---------
> > | |
> >
> > (1 row)
> >
> > Similar behavior with pg_get_logical_snapshot_info(). While the
> > existing 'pg_ls_logicalsnapdir' function gives this error, which looks
> > more meaningful:
> >
> > newdb1=# select * from pg_ls_logicalsnapdir(NULL);
> > ERROR: function pg_ls_logicalsnapdir(unknown) does not exist
> > LINE 1: select * from pg_ls_logicalsnapdir(NULL);
> > HINT: No function matches the given name and argument types. You
> > might need to add explicit type casts.
> >
> >
> > Shouldn't the new functions have same behavior?
> >
>
> No. Since the name pg_ls_logicalsnapdir has zero single-argument
> implementations passing a null value as an argument is indeed attempt to
> invoke a function signature that doesn’t exist.
Agree.
> I can see an argument that they should produce an empty set instead of a
> single all-null row,
Yeah, it's outside the scope of this patch but I've seen different behavior
in this area.
For example:
postgres=# select * from pg_ls_replslotdir(NULL);
name | size | modification
------+------+--------------
(0 rows)
as compared to:
postgres=# select * from pg_walfile_name_offset(NULL);
file_name | file_offset
-----------+-------------
|
(1 row)
I thought that it might be linked to the volatility but it is not:
postgres=# select * from pg_stat_get_xact_blocks_fetched(NULL);
pg_stat_get_xact_blocks_fetched
---------------------------------
(1 row)
postgres=# select * from pg_get_multixact_members(NULL);
xid | mode
-----+------
(0 rows)
while both are volatile.
I think both make sense: It's "empty" or we "don't know the values of the fields".
I don't know if there is any reason about this "inconsistency".
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Tue, Sep 17, 2024 at 10:46 AM David G. Johnston <david.g.johnston@gmail.com> wrote: > > > > On Monday, September 16, 2024, shveta malik <shveta.malik@gmail.com> wrote: >> >> On Tue, Sep 17, 2024 at 10:18 AM shveta malik <shveta.malik@gmail.com> wrote: >> > >> > Thanks for addressing the comments. I have not started reviewing v4 >> > yet, but here are few more comments on v3: >> > >> >> I just noticed that when we pass NULL input, both the new functions >> give 1 row as output, all cols as NULL: >> >> newdb1=# SELECT * FROM pg_get_logical_snapshot_meta(NULL); >> magic | checksum | version >> -------+----------+--------- >> | | >> >> (1 row) >> >> Similar behavior with pg_get_logical_snapshot_info(). While the >> existing 'pg_ls_logicalsnapdir' function gives this error, which looks >> more meaningful: >> >> newdb1=# select * from pg_ls_logicalsnapdir(NULL); >> ERROR: function pg_ls_logicalsnapdir(unknown) does not exist >> LINE 1: select * from pg_ls_logicalsnapdir(NULL); >> HINT: No function matches the given name and argument types. You >> might need to add explicit type casts. >> >> >> Shouldn't the new functions have same behavior? > > > No. Since the name pg_ls_logicalsnapdir has zero single-argument implementations passing a null value as an argument isindeed attempt to invoke a function signature that doesn’t exist. > > If there is exactly one single input argument function of the given name the parser is going to cast the null literal tothe data type of the single argument and invoke the function. It will not and cannot be convinced to fail to find a matchingfunction. Okay, understood. Thanks for explaining. > I can see an argument that they should produce an empty set instead of a single all-null row, but the idea that they wouldn’teven be found is contrary to a core design of the system. Okay, a single row can be investigated if it comes under this scope. But I see why 'ERROR' is not a possibility here. thanks Shveta
On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Sep 17, 2024 at 10:18:35AM +0530, shveta malik wrote: > > Thanks for addressing the comments. I have not started reviewing v4 > > yet, but here are few more comments on v3: > > > > 1) > > +#include "port/pg_crc32c.h" > > > > It is not needed in pg_logicalinspect.c as it is already included in > > internal_snapbuild.h > > Yeap, forgot to remove that one when creating the new "internal".h file, done > in v5 attached, thanks! > > > > > 2) > > + values[0] = Int16GetDatum(ondisk.builder.state); > > ........ > > + values[8] = LSNGetDatum(ondisk.builder.last_serialized_snapshot); > > + values[9] = TransactionIdGetDatum(ondisk.builder.next_phase_at); > > + values[10] = Int64GetDatum(ondisk.builder.committed.xcnt); > > > > We can have values[i++] in all the places and later we can check : > > Assert(i == PG_GET_LOGICAL_SNAPSHOT_INFO_COLS); > > Then we need not to keep track of number even in later part of code, > > as it goes till 14. > > Right, let's do it that way (as it is done in pg_walinspect for example). > > > 4) > > Most of the output columns in pg_get_logical_snapshot_info() look > > self-explanatory except 'state'. Should we have meaningful 'text' here > > corresponding to SnapBuildState? Similar to what we do for > > 'invalidation_reason' in pg_replication_slots. (SlotInvalidationCauses > > for ReplicationSlotInvalidationCause) > > Yeah we could. I was not sure about that (and that was my first remark in [1]) > , as the module is mainly for debugging purpose, I was thinking that the one > using it could refer to "snapbuild.h". Let's see what others think. > okay, makes sense. lets wait what others have to say. Thanks for the patch. Few trivial things: 1) May be we shall change 'INTERNAL_SNAPBUILD_H' in snapbuild_internal.h to 'SNAPBUILD_INTERNAL_H'? 2) ValidateSnapshotFile() It is not only validating, but loading the content as well. So may be we can rename to ValidateAndRestoreSnapshotFile? 3) sgml: a) + The pg_logicalinspect functions are called using an LSN argument that can be extracted from the output name of the pg_ls_logicalsnapdir() function. Is it possible to give link to pg_ls_logicalsnapdir function here? b) + Gets logical snapshot metadata about a snapshot file that is located in the pg_logical/snapshots directory. located in server's pg_logical/snapshots directory (i.e. use server keyword, similar to how pg_ls_logicalsnapdir , pg_ls_logicalmapdir explains it) thanks Shveta
On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > On Tue, Sep 17, 2024 at 10:18:35AM +0530, shveta malik wrote: > > Thanks for addressing the comments. I have not started reviewing v4 > > yet, but here are few more comments on v3: > > > > > 4) > > Most of the output columns in pg_get_logical_snapshot_info() look > > self-explanatory except 'state'. Should we have meaningful 'text' here > > corresponding to SnapBuildState? Similar to what we do for > > 'invalidation_reason' in pg_replication_slots. (SlotInvalidationCauses > > for ReplicationSlotInvalidationCause) > > Yeah we could. I was not sure about that (and that was my first remark in [1]) > , as the module is mainly for debugging purpose, I was thinking that the one > using it could refer to "snapbuild.h". Let's see what others think. > Displaying the 'text' for the state column makes it easy to understand. So, +1 for doing it that way. -- With Regards, Amit Kapila.
On Thu, Sep 19, 2024 at 12:17 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > Thanks for the review! > Thanks for the patch. Should we include in the document who can execute these functions and the required access permissions, similar to how it's done for pgwalinspect, pg_ls_logicalmapdir(), and other such functions? thanks Shveta
On Fri, Sep 20, 2024 at 12:22 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
>
> Please find attached v8, that:
>
Thank You for the patch. In one of my tests, I noticed that I got
negative checksum:
postgres=# SELECT * FROM pg_get_logical_snapshot_meta('0/3481F20');
magic | checksum | version
------------+------------+---------
1369563137 | -266346460 | 6
But pg_crc32c is uint32. Is it because we are getting it as
Int32GetDatum(ondisk.checksum) in pg_get_logical_snapshot_meta()?
Instead should it be UInt32GetDatum?
Same goes for below:
values[i++] = Int32GetDatum(ondisk.magic);
values[i++] = Int32GetDatum(ondisk.magic);
We need to recheck the rest of the fields in the info() function as well.
thanks
Shveta
On Mon, Sep 23, 2024 at 7:57 AM Peter Smith <smithpb2250@gmail.com> wrote:
>
> My review comments for v8-0001
>
> ======
> contrib/pg_logicalinspect/pg_logicalinspect.c
>
> 1.
> +/*
> + * Lookup table for SnapBuildState.
> + */
> +
> +#define SNAPBUILD_STATE_INCR 1
> +
> +static const char *const SnapBuildStateDesc[] = {
> + [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
> + [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
> + [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
> + [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
> +};
> +
> +/*
>
> nit - the SNAPBUILD_STATE_INCR made this code appear more complicated
> than it is.
I agree.
> Please take a look at the attachment for an alternative
> implementation which includes an explanatory comment. YMMV. Feel free
> to ignore it.
>
+1. I find Peter's version with comments easier to understand.
thanks
Shveta
Hi,
On Tue, Sep 24, 2024 at 09:15:31AM +0530, shveta malik wrote:
> On Fri, Sep 20, 2024 at 12:22 PM Bertrand Drouvot
> <bertranddrouvot.pg@gmail.com> wrote:
> >
> >
> > Please find attached v8, that:
> >
>
> Thank You for the patch. In one of my tests, I noticed that I got
> negative checksum:
>
> postgres=# SELECT * FROM pg_get_logical_snapshot_meta('0/3481F20');
> magic | checksum | version
> ------------+------------+---------
> 1369563137 | -266346460 | 6
>
> But pg_crc32c is uint32. Is it because we are getting it as
> Int32GetDatum(ondisk.checksum) in pg_get_logical_snapshot_meta()?
> Instead should it be UInt32GetDatum?
Thanks for the testing.
As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it
to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on
the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum().
> Same goes for below:
> values[i++] = Int32GetDatum(ondisk.magic);
> values[i++] = Int32GetDatum(ondisk.magic);
The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is
making use of UInt32GetDatum() and keep int4 in the sql file.
> We need to recheck the rest of the fields in the info() function as well.
I think that the pg_get_logical_snapshot_info()'s fields are ok (I did spend some
time to debug CI failing on the 32bit build for some on them before submitting v1).
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Wed, Sep 25, 2024 at 2:51 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Sep 23, 2024 at 12:27:27PM +1000, Peter Smith wrote:
> > My review comments for v8-0001
> >
> > ======
> > contrib/pg_logicalinspect/pg_logicalinspect.c
> >
> > 1.
> > +/*
> > + * Lookup table for SnapBuildState.
> > + */
> > +
> > +#define SNAPBUILD_STATE_INCR 1
> > +
> > +static const char *const SnapBuildStateDesc[] = {
> > + [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
> > + [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
> > + [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
> > + [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
> > +};
> > +
> > +/*
> >
> > nit - the SNAPBUILD_STATE_INCR made this code appear more complicated
> > than it is. Please take a look at the attachment for an alternative
> > implementation which includes an explanatory comment. YMMV. Feel free
> > to ignore it.
>
> Thanks for the feedback!
>
> I like the commment, so added it in v9 attached. OTOH I think that's better
> to keep SNAPBUILD_STATE_INCR as those "+1" are all linked and that would be
> easy to miss the one in pg_get_logical_snapshot_info() should we change the
> increment in the future.
>
I see SNAPBUILD_STATE_INCR more as an "offset" (to get the lowest enum
value to be at lookup index [0]) than an "increment" (between the enum
values), so I'd be naming that differently. But, maybe I am straying
into just personal opinion instead of giving useful feedback, so let's
say I have no more review comments. Patch v9 looks OK to me.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
On Tue, Sep 24, 2024 at 10:23 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Tue, Sep 24, 2024 at 09:15:31AM +0530, shveta malik wrote:
> > On Fri, Sep 20, 2024 at 12:22 PM Bertrand Drouvot
> > <bertranddrouvot.pg@gmail.com> wrote:
> > >
> > >
> > > Please find attached v8, that:
> > >
> >
> > Thank You for the patch. In one of my tests, I noticed that I got
> > negative checksum:
> >
> > postgres=# SELECT * FROM pg_get_logical_snapshot_meta('0/3481F20');
> > magic | checksum | version
> > ------------+------------+---------
> > 1369563137 | -266346460 | 6
> >
> > But pg_crc32c is uint32. Is it because we are getting it as
> > Int32GetDatum(ondisk.checksum) in pg_get_logical_snapshot_meta()?
> > Instead should it be UInt32GetDatum?
>
> Thanks for the testing.
>
> As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it
> to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on
> the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum().
>
Okay, looks good,
> > Same goes for below:
> > values[i++] = Int32GetDatum(ondisk.magic);
> > values[i++] = Int32GetDatum(ondisk.magic);
>
> The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is
> making use of UInt32GetDatum() and keep int4 in the sql file.
>
> > We need to recheck the rest of the fields in the info() function as well.
>
> I think that the pg_get_logical_snapshot_info()'s fields are ok (I did spend some
> time to debug CI failing on the 32bit build for some on them before submitting v1).
>
+ OUT catchange_xip xid[]
One question, what is xid datatype, is it too int8? Sorry, could not
find the correct doc. Since we are getting uint32 in Int64, this also
needs to be accordingly.
thanks
Shveta
Is there a reason for this elaborate error handling:
+ fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
+
+ if (fd < 0 && errno == ENOENT)
+ ereport(ERROR,
+ errmsg("file \"%s\" does not exist", path));
+ else if (fd < 0)
+ ereport(ERROR,
+ (errcode_for_file_access(),
+ errmsg("could not open file \"%s\": %m", path)));
Couldn't you just use the second branch for all errno's?
Hi, On Wed, Sep 25, 2024 at 11:23:17AM +0530, shveta malik wrote: > + OUT catchange_xip xid[] > > One question, what is xid datatype, is it too int8? Sorry, could not > find the correct doc. I think that we can get the answer from pg_type: postgres=# select typname,typlen from pg_type where typname = 'xid'; typname | typlen ---------+-------- xid | 4 (1 row) > Since we are getting uint32 in Int64, this also needs to be accordingly. I think the way it is currently done is fine: we're dealing with TransactionId (and not with FullTransactionId). So, the Int64GetDatum() output would still stay in the "xid" range. Keeping xid in the .sql makes it clear that we are dealing with transaction ID. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
Hi,
On Wed, Sep 25, 2024 at 10:29 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Wed, Sep 25, 2024 at 04:04:43PM +0200, Peter Eisentraut wrote:
> > Is there a reason for this elaborate error handling:
>
> Thanks for looking at it!
>
> > + fd = OpenTransientFile(path, O_RDONLY | PG_BINARY);
> > +
> > + if (fd < 0 && errno == ENOENT)
> > + ereport(ERROR,
> > + errmsg("file \"%s\" does not exist", path));
> > + else if (fd < 0)
> > + ereport(ERROR,
> > + (errcode_for_file_access(),
> > + errmsg("could not open file \"%s\": %m", path)));
> >
> > Couldn't you just use the second branch for all errno's?
>
> Yeah, I think it comes from copying/pasting from SnapBuildRestore() too "quickly".
> v10 attached uses the second branch only.
>
I've reviewed v10 patch and here are some comments:
+static void
+ValidateAndRestoreSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
+ const char *path)
This function and SnapBuildRestore() have duplicate codes. Can we have
a common function that reads the snapshot data from disk to
SnapBuildOnDisk, and have both ValidateAndRestoreSnapshotFile() and
SnapBuildRestore() call it?
---
+CREATE FUNCTION pg_get_logical_snapshot_meta(IN in_lsn pg_lsn,
(snip)
+CREATE FUNCTION pg_get_logical_snapshot_info(IN in_lsn pg_lsn,
Is there any reason why both functions take a pg_lsn value as an
argument? Given that the main usage would be to use these functions
with pg_ls_logicalsnapdir(), I think it would be easier for users if
these functions take a filename as a function argument. That way, we
can use these functions like:
select info.* from pg_ls_logicalsnapdir(),
pg_get_logical_snapshot_info(name) as info;
If there are use cases where specifying a LSN is easier, I think we
would have both types.
----
+static const char *const SnapBuildStateDesc[] = {
+ [SNAPBUILD_START + SNAPBUILD_STATE_INCR] = "start",
+ [SNAPBUILD_BUILDING_SNAPSHOT + SNAPBUILD_STATE_INCR] = "building",
+ [SNAPBUILD_FULL_SNAPSHOT + SNAPBUILD_STATE_INCR] = "full",
+ [SNAPBUILD_CONSISTENT + SNAPBUILD_STATE_INCR] = "consistent",
+};
I think that it'd be better to have a dedicated function that returns
a string representation of the given state by using a switch
statement. That way, we don't need SNAPBUILD_STATE_INCR and a compiler
warning would help realize a missing state if a new state is
introduced in the future. It needs a function call but I believe it
won't be a problem in this use case.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, Sep 25, 2024 at 11:01 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Wed, Sep 25, 2024 at 11:23:17AM +0530, shveta malik wrote: > > + OUT catchange_xip xid[] > > > > One question, what is xid datatype, is it too int8? Sorry, could not > > find the correct doc. > > I think that we can get the answer from pg_type: > > postgres=# select typname,typlen from pg_type where typname = 'xid'; > typname | typlen > ---------+-------- > xid | 4 > (1 row) > > > Since we are getting uint32 in Int64, this also needs to be accordingly. > > I think the way it is currently done is fine: we're dealing with TransactionId > (and not with FullTransactionId). So, the Int64GetDatum() output would still > stay in the "xid" range. Keeping xid in the .sql makes it clear that we are > dealing with transaction ID. > Okay, got it. The 'xid' usage is fine then. thanks Shveta
On Tue, Oct 8, 2024 at 9:25 AM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Tue, Oct 08, 2024 at 04:25:29PM +1100, Peter Smith wrote:
> > Hi, here are some review comments for patch v11.
>
> Thanks for looking at it!
>
> > ======
> > contrib/pg_logicalinspect/specs/logical_inspect.spec
> >
> > 1.
> > nit - Add some missing spaces after commas (,) in the SQL.
>
> Fine by me, done in v12 attached.
>
> > ======
> > doc/src/sgml/pglogicalinspect.sgml
> >
> > 2.
> > + <note>
> > + <para>
> > + The <filename>pg_logicalinspect</filename> functions are called
> > + using a text argument that can be extracted from the output name of the
> > + <function>pg_ls_logicalsnapdir</function>() function.
> > + </para>
> > + </note>
> >
> > 2a. wording
> >
> > The wording "using a text argument that can be extracted" seems like a
> > hangover from the previous implementation; it does not even say what
> > that "text argument" means.
>
> That's right (it's mentioned later on (for each function description) that
> the argument represents the snapshot file name though).
>
> > Why not just say it is a snapshot
> > filename, something like below?
> >
> > SUGGESTION:
> > The pg_logicalinspect functions are called passing a snapshot filename
> > to be inspected. For example, pass a name obtained from the
> > pg_ls_logicalsnapdir() function.
>
> Yeah, I like it, but...
>
> > ~
> >
> > 2b. formatting
> >
> > nit - In the previous implementation the extraction of the LSN was
> > trickier, so this part was worthy of an SGML "NOTE". Now that it is
> > just a filename, I don't know if it needs to be a special note
> > anymore.
>
> In fact, giving it more thoughts, I think we can just remove this part.
> I don't see the extra value anymore and that's something that we may need to
> remove depending on what will be added to this module in the future.
>
> I think that having the argument explanation in each function description is
> enough, done that way in v12.
>
> >
> > ~~~
> >
> > 3.
> > +postgres=# SELECT meta.* FROM pg_ls_logicalsnapdir(),
> > +pg_get_logical_snapshot_meta(name) AS meta;
> > +
> > +-[ RECORD 1 ]--------
> > +magic | 1369563137
> > +checksum | 1028045905
> > +version | 6
> >
> > 3a.
> > If you are going to wrap the SQL across multiple lines like this, then
> > you should show the psql continuation prompt, so that the example
> > looks the same as what the user would see.
>
> I'm not sure about this one. If the user copy/paste the doc as it is then there
> is no psql continuation prompt. If the user does not copy/paste the doc then he
> might indeed see "something" else (but that's not surprising since he did not
> copy/paste). FWIW, there is similar examples in pgstatstatements.sgml.
>
> > ~
> >
> > 3b.
> > FYI, the output of that can return multiple records,
>
> Yes, as the test in this patch does.
>
> > which is
> > b.i) probably not what you intended to demonstrate
> > b.ii) not the same as what the example says
> >
> > e.g., I got this:
> > test_pub=# SELECT meta.* FROM pg_ls_logicalsnapdir(),
> > test_pub-# pg_get_logical_snapshot_meta(name) AS meta;
> > -[ RECORD 1 ]--------
> > magic | 1369563137
> > checksum | 681884630
> > version | 6
> > -[ RECORD 2 ]--------
> > magic | 1369563137
> > checksum | 2213048308
> > version | 6
> > -[ RECORD 3 ]--------
> > magic | 1369563137
> > checksum | 3812680762
> > version | 6
> > -[ RECORD 4 ]--------
> > magic | 1369563137
> > checksum | 3759893001
> > version | 6
> >
>
> I don't get the point here. The examples just show another way to use the functions,
> the ouput is more "anecdotal" than anything else.
>
> >
> > ~~~
> >
> > (Also those #3a, #3b comments apply to both examples)
> >
> > ======
> > src/backend/replication/logical/snapbuild.c
> >
> > 4.
> > - SnapBuild builder;
> > -
> > - /* variable amount of TransactionIds follows */
> > -} SnapBuildOnDisk;
> > -
> > #define SnapBuildOnDiskConstantSize \
> > offsetof(SnapBuildOnDisk, builder)
> > #define SnapBuildOnDiskNotChecksummedSize \
> >
> > Is it better to try to keep those "Size" macros defined along with
> > wherever the SnapBuildOnDisk is defined? Otherwise, if the structure
> > is ever changed, adjusting the macros could be easily overlooked.
>
> I think that the less we put in the snapbuild_internal.h the better. That said,
> I think you have a good point so I added a comment around the SnapBuildOnDisk
> definition instead in v12.
>
> >
> > ~~~
> >
> > 5.
> > ValidateAndRestoreSnapshotFile
> >
> > nit - See [1] #4 suggestion to declare 'sz' at scope where used. The
> > previous reason not to change this (e.g. "mainly inspired from
> > SnapBuildRestore") seems less relevant because now most lines of this
> > function have already been modified for other reasons.
>
> Right. I think that's a matter of taste and I do prefer to "only" do the
> necessary changes that are linked to the feature the patch is implementing.
>
> > ~~~
> >
> > 6.
> > SnapBuildRestore:
> >
> > + if (fd < 0 && errno == ENOENT)
> > + return false;
> > + else if (fd < 0)
> > + ereport(ERROR,
> > + (errcode_for_file_access(),
> > + errmsg("could not open file \"%s\": %m", path)));
> >
> > I think this code fragment looked like this before, and you only
> > relocated it,
>
> That's right.
>
> > but it still seems a bit awkward to write this way.
> > Since so much else has changed, how about also improving this in
> > passing, like below:
> >
> > if (fd < 0)
> > {
> > if (errno == ENOENT)
> > return false;
> >
> > ereport(ERROR,
> > (errcode_for_file_access(),
> > errmsg("could not open file \"%s\": %m", path)));
> > }
>
> Same, I do prefer to "only" do the necessary changes that are linked to the
> feature the patch is implementing (and why stop here, a similar change could be
> made in logical/origin.c too for example).
>
Thank you for updating the patch! I have some comments on v12 patch:
---
+ 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 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));
---
+# 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.
---
+# Generated subdirectories
+/log/
+/results/
+/tmp_check/
If we need to use the isolation tests (see above comment), we need to
add both output_iso and tmp_check_iso as well.
---
+ 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.
---
+ 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(). 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)));
}
:
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com
On Wed, Oct 9, 2024 at 3:25 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Tue, Oct 08, 2024 at 04:25:29PM +1100, Peter Smith wrote: > > 3. > > +postgres=# SELECT meta.* FROM pg_ls_logicalsnapdir(), > > +pg_get_logical_snapshot_meta(name) AS meta; > > + > > +-[ RECORD 1 ]-------- > > +magic | 1369563137 > > +checksum | 1028045905 > > +version | 6 > > > > 3a. > > If you are going to wrap the SQL across multiple lines like this, then > > you should show the psql continuation prompt, so that the example > > looks the same as what the user would see. > > I'm not sure about this one. If the user copy/paste the doc as it is then there > is no psql continuation prompt. If the user does not copy/paste the doc then he > might indeed see "something" else (but that's not surprising since he did not > copy/paste). FWIW, there is similar examples in pgstatstatements.sgml. > > > ~ > > > > 3b. > > FYI, the output of that can return multiple records, > > Yes, as the test in this patch does. > > > which is > > b.i) probably not what you intended to demonstrate > > b.ii) not the same as what the example says > > > > e.g., I got this: > > test_pub=# SELECT meta.* FROM pg_ls_logicalsnapdir(), > > test_pub-# pg_get_logical_snapshot_meta(name) AS meta; > > -[ RECORD 1 ]-------- > > magic | 1369563137 > > checksum | 681884630 > > version | 6 > > -[ RECORD 2 ]-------- > > magic | 1369563137 > > checksum | 2213048308 > > version | 6 > > -[ RECORD 3 ]-------- > > magic | 1369563137 > > checksum | 3812680762 > > version | 6 > > -[ RECORD 4 ]-------- > > magic | 1369563137 > > checksum | 3759893001 > > version | 6 > > > > I don't get the point here. The examples just show another way to use the functions, > the ouput is more "anecdotal" than anything else. > I mistakenly thought the purpose of the third part of the example was to give a shorthand way of doing the same as the first two parts -- so, using one SQL query instead of two. But unless this SQL is modified also to output the name of the/each snapfile then I'm not sure how these 3rd part examples are particularly useful. e.g. Without an associated filename, all this query will yield is a bunch of meta-data (or info-data) records but you have no idea which snapshots they belong to. How about doing this: SELECT ss.name, info.* FROM pg_ls_logicalsnapdir() AS ss, pg_get_logical_snapshot_info(ss.name) AS info; ====== Kind Regards, Peter Smith. Fujitsu Australia
Hi, On Wed, Oct 09, 2024 at 11:41:44AM +1100, Peter Smith wrote: > On Wed, Oct 9, 2024 at 3:25 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > > > I don't get the point here. The examples just show another way to use the functions, > > the ouput is more "anecdotal" than anything else. > > > > How about doing this: > SELECT ss.name, info.* FROM pg_ls_logicalsnapdir() AS ss, > pg_get_logical_snapshot_info(ss.name) AS info; Agree that it makes sense to add the snapshot file name, will add in v13, thanks! Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
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
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
Hi, On Wed, Oct 09, 2024 at 10:21:31AM -0700, Masahiko Sawada wrote: > On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot > <bertranddrouvot.pg@gmail.com> wrote: > > 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? I had a closer look to d746021de1 (which introduced construct_array_builtin()) and the hackers thread that lead to it [1]. IIUC, the idea was to: 1. centralize the hardcoded knowledge that were in the calls to construct_array() and deconstruct_array() for built-in types 2. notational simplification and bug-proofing As XIDOID is a built-in type, I think that it would make sense to add it in deconstruct_array_builtin()/construct_array_builtin(). I think the reason XIDOID has not been added in d746021de1 is that there were no use case at that time (means no existing calls to construct_array()/deconstruct_array() with hardcoded XIDOID related arguments). One could say that we would just add 2 calls to construct_array() in the pg_logicalinspect module but, for example, d746021de1 also took care of CSTRINGOID that had a single call at that time: $ git show d746021de1 | grep deconstruct_array_builtin | grep -c CSTRINGOID 1 $ git show d746021de1 | grep construct_array_builtin | grep -v deconstruct_array_builtin | grep -c CSTRINGOID 1 So I think that having construct_array_builtin()/deconstruct_array_builtin() taking care of XIDOID is the way to go. If that makes sense to you then I'll submit a dedicated patch for it, thoughts? [1]: https://www.postgresql.org/message-id/flat/2914356f-9e5f-8c59-2995-5997fc48bcba%40enterprisedb.com Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Wed, Oct 09, 2024 at 10:21:31AM -0700, Masahiko Sawada wrote: > > On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > 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? > > I had a closer look to d746021de1 (which introduced construct_array_builtin()) > and the hackers thread that lead to it [1]. > > IIUC, the idea was to: > > 1. centralize the hardcoded knowledge that were in the calls to construct_array() > and deconstruct_array() for built-in types > 2. notational simplification and bug-proofing > > As XIDOID is a built-in type, I think that it would make sense to add it in > deconstruct_array_builtin()/construct_array_builtin(). > > I think the reason XIDOID has not been added in d746021de1 is that there were no > use case at that time (means no existing calls to construct_array()/deconstruct_array() > with hardcoded XIDOID related arguments). > > One could say that we would just add 2 calls to construct_array() in the pg_logicalinspect > module but, for example, d746021de1 also took care of CSTRINGOID that had a single > call at that time: > > $ git show d746021de1 | grep deconstruct_array_builtin | grep -c CSTRINGOID > 1 > $ git show d746021de1 | grep construct_array_builtin | grep -v deconstruct_array_builtin | grep -c CSTRINGOID > 1 > > So I think that having construct_array_builtin()/deconstruct_array_builtin() > taking care of XIDOID is the way to go. If that makes sense to you then I'll > submit a dedicated patch for it, thoughts? Your explanation makes sense to me. I think it can be included in the main pg_logicalinspect patch as this change is a part of it. Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Thu, Oct 10, 2024 at 12:05:10AM -0700, Masahiko Sawada wrote: > > On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > So I think that having construct_array_builtin()/deconstruct_array_builtin() > > > taking care of XIDOID is the way to go. If that makes sense to you then I'll > > > submit a dedicated patch for it, thoughts? > > > > Your explanation makes sense to me. > > Thanks for sharing your thoughts. > > > I think it can be included in the main pg_logicalinspect patch as this change > > is a part of it. > > Okay, let's keep the discussion here. Please find attached v13 that takes care > of your previous remarks and Peter's one ([1]). > > FYI, v13 is splitted into 2 sub-patches (0001 for the discussion related to > XIDOID and construct_array_builtin() and 0002 for the module itself). Thank you for updating the patch! > > FWIW, the elmbyval and elmalign values that are added in 0001 have been deduced > from: > > postgres=# select typbyval, typalign from pg_type where typname = 'xid'; > typbyval | typalign > ----------+---------- > t | i > (1 row) +1 The patches mostly look good to me. Here are some minor comments: + sprintf(path, "%s/%s", + PG_LOGICAL_SNAPSHOTS_DIR, + text_to_cstring(filename_t)); + + /* Validate and restore the snapshot to 'ondisk' */ + ValidateAndRestoreSnapshotFile(&ondisk, path, CurrentMemoryContext, false); + + /* Build a tuple descriptor for our result type */ + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) + elog(ERROR, "return type must be a row type"); + I think it would be better to check the result type before reading the snapshot file. --- + values[i++] = Int64GetDatum((int64) ondisk.checksum); Why is only checksum casted to int64? With that, it can show a checksum value as a non-netagive integer but is it really necessary? For instance, page_header() function in pageinspect shows a page checksum as smallint. ---@@ -0,0 +1,4 @@ +# Generated subdirectories +/log/ +/results/ +/tmp_check/ output_iso and tmp_check_iso should be added. --- /* - * Restore a snapshot into 'builder' if previously one has been stored at the - * location indicated by 'lsn'. Returns true if successful, false otherwise. + * Validate the logical snapshot file and read its contents to 'ondisk'. */ -static bool -SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn) +bool +ValidateAndRestoreSnapshotFile(SnapBuildOnDisk *ondisk, const char *path, + MemoryContext context, bool missing_ok) I think it would be better to add some descriptions of the function arguments, particularly context and missing_ok. The names of all other functions in snapbuild.c have the "SnapBuild" prefix. I think it's better to follow it. How about renaming it to SnapBuildReadSnapshot(), SnapBuildRestoreSnapshot(), or something along those lines? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Hi, Here are a few comments for patch set v13*
//////////
Patch v13-0001
======
Commit message
1.1
/were no use case/was no use case/
~~~
1.2
It seemed a bit odd that the switch cases for
'construct_array_builtin' are not the same as those for
'deconstruct_array_builtin'.
For example, all these ones seem missing from deconstruct:
case INT4OID:
case INT8OID:
case NAMEOID:
case REGTYPEOID:
I know that has nothing to do with your patch, and I guess it does not
cause any problems otherwise there would be ERRORs. But, if you are to
follow this same current pattern, then perhaps you don't need to add
your new case for 'deconstruct_array_builtin', since AFAICT you are
never using it.
//////////
Patch v13-0002
======
pg_get_logical_snapshot_meta:
2.1
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
FWIW, if you wanted to avoid a few lines you could initialise the
nulls array during the declaration.
bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
This seems a common pattern in other source code, and it replaces the
need for the subsequent memset.
~~~
pg_get_logical_snapshot_info:
2.2
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS];
Ditto of #2.1. You could instead just initialise in the declaration like:
bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi,
On Fri, Oct 11, 2024 at 12:59:33PM +1100, Peter Smith wrote:
> Hi, Here are a few comments for patch set v13*
Thanks for looking at it.
> //////////
>
> Patch v13-0001
>
> ======
> Commit message
>
> 1.1
> /were no use case/was no use case/
Updated in v14 just shared up-thread.
> ~~~
>
> 1.2
> It seemed a bit odd that the switch cases for
> 'construct_array_builtin' are not the same as those for
> 'deconstruct_array_builtin'.
>
> For example, all these ones seem missing from deconstruct:
> case INT4OID:
> case INT8OID:
> case NAMEOID:
> case REGTYPEOID:
>
> I know that has nothing to do with your patch, and I guess it does not
> cause any problems otherwise there would be ERRORs. But, if you are to
> follow this same current pattern, then perhaps you don't need to add
> your new case for 'deconstruct_array_builtin', since AFAICT you are
> never using it.
That's right. Strict pairing between deconstruct_array_builtin() and
construct_array_builtin() is not required, let's remove this extra switch in
deconstruct_array_builtin() for code consistency (done in v14).
> //////////
>
> Patch v13-0002
>
> ======
> pg_get_logical_snapshot_meta:
>
> 2.1
> + Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
> + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS];
>
> FWIW, if you wanted to avoid a few lines you could initialise the
> nulls array during the declaration.
> bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
>
> This seems a common pattern in other source code, and it replaces the
> need for the subsequent memset.
Okay, fine by me, let's do it for the "values" too in passing (this seems also
a common pattern), in v14.
Regards,
--
Bertrand Drouvot
PostgreSQL Contributors Team
RDS Open Source Databases
Amazon Web Services: https://aws.amazon.com
On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot <bertranddrouvot.pg@gmail.com> wrote: > > Hi, > > On Thu, Oct 10, 2024 at 05:38:43PM -0700, Masahiko Sawada wrote: > > On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > The patches mostly look good to me. Here are some minor comments: > > Thanks for looking at it! > > > > > + sprintf(path, "%s/%s", > > + PG_LOGICAL_SNAPSHOTS_DIR, > > + text_to_cstring(filename_t)); > > + > > + /* Validate and restore the snapshot to 'ondisk' */ > > + ValidateAndRestoreSnapshotFile(&ondisk, path, > > CurrentMemoryContext, false); > > + > > + /* Build a tuple descriptor for our result type */ > > + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) > > + elog(ERROR, "return type must be a row type"); > > + > > I think it would be better to check the result type before reading the > > snapshot file. > > Agree, done in v14. > > > > > --- > > + values[i++] = Int64GetDatum((int64) ondisk.checksum); > > > > Why is only checksum casted to int64? With that, it can show a > > checksum value as a non-netagive integer but is it really necessary? > > For instance, page_header() function in pageinspect shows a page > > checksum as smallint. > > Yeah, pd_checksum in PageHeaderData is uint16 while checksum in SnapBuildOnDisk > is pg_crc32c. The reason why it is casted to int64 is explained in [1], does that > make sense to you? In the email, you said: > As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it > to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on > the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum(). I'm fine with using Int64GetDatum() for checksum. > > > Same goes for below: > > values[i++] = Int32GetDatum(ondisk.magic); > > values[i++] = Int32GetDatum(ondisk.magic); > > The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is > making use of UInt32GetDatum() and keep int4 in the sql file. While I agree that these two fields are unlikely to be > 2^31 - 1, I'm concerned a bit about an inconsistency that the patch uses Int64GetDatum also for both ondisk.builder.committed.xcnt and ondisk.builder.catchange.xcnt. I have a minor comment: + <sect2 id="pglogicalinspect-funcs"> + <title>General Functions</title> If we use "General Functions" here it sounds like there are other functions for specific purposes in pg_logicalinspect module. How about using "Functions" instead? Regards, -- Masahiko Sawada Amazon Web Services: https://aws.amazon.com
Here are some minor review comments for v15-0002.
======
contrib/pg_logicalinspect/pg_logicalinspect.c
1.
+pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_META_COLS 3
+ SnapBuildOnDisk ondisk;
+ HeapTuple tuple;
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
+ TupleDesc tupdesc;
+ char path[MAXPGPATH];
+ int i = 0;
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+
+ sprintf(path, "%s/%s",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ text_to_cstring(filename_t));
+
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
+
+ /* Validate and restore the snapshot to 'ondisk' */
+ SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
The sprintf should be deferred. Could you do it after the ERROR check?
~~~
2.
+pg_get_logical_snapshot_info(PG_FUNCTION_ARGS)
+{
+#define PG_GET_LOGICAL_SNAPSHOT_INFO_COLS 14
+ SnapBuildOnDisk ondisk;
+ HeapTuple tuple;
+ Datum values[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+ bool nulls[PG_GET_LOGICAL_SNAPSHOT_INFO_COLS] = {0};
+ TupleDesc tupdesc;
+ char path[MAXPGPATH];
+ int i = 0;
+ text *filename_t = PG_GETARG_TEXT_PP(0);
+
+ sprintf(path, "%s/%s",
+ PG_LOGICAL_SNAPSHOTS_DIR,
+ text_to_cstring(filename_t));
+
+ /* Build a tuple descriptor for our result type */
+ if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
+ elog(ERROR, "return type must be a row type");
Ditto #1. The sprintf should be deferred. Could you do it after the ERROR check?
======
src/backend/replication/logical/snapbuild.c
3.
/*
- * Restore a snapshot into 'builder' if previously one has been stored at the
- * location indicated by 'lsn'. Returns true if successful, false otherwise.
+ * Restore the logical snapshot file contents to 'ondisk'.
+ *
+ * If 'missing_ok' is true, will not throw an error if the file is not found.
+ * 'context' is the memory context where the catalog modifying/committed xid
+ * will live.
*/
-static bool
-SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
+bool
+SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
+ MemoryContext context, bool missing_ok)
nit - I think it's better to describe parameters in the same order
that they are declared. Also, include a 'path' description, so it is
not the only one omitted.
SUGGESTION:
'path' - snapshot file path.
'context' - memory context where the catalog modifying/committed xid will live.
‘missing_ok’ – when true, don't throw an error if the file is not found.
======
Kind Regards,
Peter Smith.
Fujitsu Australia
Hi, On Fri, Oct 11, 2024 at 04:48:26PM -0700, Masahiko Sawada wrote: > On Fri, Oct 11, 2024 at 11:15 AM Masahiko Sawada <sawada.mshk@gmail.com> wrote: > > > > On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > Hi, > > > > > > On Thu, Oct 10, 2024 at 05:38:43PM -0700, Masahiko Sawada wrote: > > > > On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot > > > > <bertranddrouvot.pg@gmail.com> wrote: > > > > > > > > The patches mostly look good to me. Here are some minor comments: > > > > > > Thanks for looking at it! > > > > > > > > > > > + sprintf(path, "%s/%s", > > > > + PG_LOGICAL_SNAPSHOTS_DIR, > > > > + text_to_cstring(filename_t)); > > > > + > > > > + /* Validate and restore the snapshot to 'ondisk' */ > > > > + ValidateAndRestoreSnapshotFile(&ondisk, path, > > > > CurrentMemoryContext, false); > > > > + > > > > + /* Build a tuple descriptor for our result type */ > > > > + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE) > > > > + elog(ERROR, "return type must be a row type"); > > > > + > > > > I think it would be better to check the result type before reading the > > > > snapshot file. > > > > > > Agree, done in v14. > > > > > > > > > > > --- > > > > + values[i++] = Int64GetDatum((int64) ondisk.checksum); > > > > > > > > Why is only checksum casted to int64? With that, it can show a > > > > checksum value as a non-netagive integer but is it really necessary? > > > > For instance, page_header() function in pageinspect shows a page > > > > checksum as smallint. > > > > > > Yeah, pd_checksum in PageHeaderData is uint16 while checksum in SnapBuildOnDisk > > > is pg_crc32c. The reason why it is casted to int64 is explained in [1], does that > > > make sense to you? > > > > In the email, you said: > > > > > As the checksum could be > 2^31 - 1, then v9 (just shared up-thread) changes it > > > to an int8 in the pg_logicalinspect--1.0.sql file. So, to avoid CI failure on > > > the 32bit build, then v9 is using Int64GetDatum() instead of UInt32GetDatum(). > > > > I'm fine with using Int64GetDatum() for checksum. > > > > > > > > > Same goes for below: > > > > values[i++] = Int32GetDatum(ondisk.magic); > > > > values[i++] = Int32GetDatum(ondisk.magic); > > > > > > The 2 others field (magic and version) are unlikely to be > 2^31 - 1, so v9 is > > > making use of UInt32GetDatum() and keep int4 in the sql file. > > > > While I agree that these two fields are unlikely to be > 2^31 - 1, I'm > > concerned a bit about an inconsistency that the patch uses > > Int64GetDatum also for both ondisk.builder.committed.xcnt and > > ondisk.builder.catchange.xcnt. Thanks for the feedback. That makes sense and I agree with the proposal done in v15. > > > > I have a minor comment: > > > > + <sect2 id="pglogicalinspect-funcs"> > > + <title>General Functions</title> > > > > If we use "General Functions" here it sounds like there are other > > functions for specific purposes in pg_logicalinspect module. How about > > using "Functions" instead? > > To elaborate further, pageinspect has a "General Functions" section, > which makes sense to me as it has other AM-type specific functions. On > the other hand, pg_logicalinspect has SQL functions only for one > logical replication component. So I think it makes sense to use > "Function" instead. pg_walinspect also has the sole section "General > Function" Yeah, I used it as a "template". > but I personally think that "Function" is more appropriate > like other modules does. I do agree. > BTW I think that adding snapshot_internal.h could be a separate patch. > That makes the main pg_logicalinspect patch cleaner. Agree. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
FYI - Although I did not re-apply/test the latest patchset v16*, by visual inspection of the minor v15/v16 diffs it looks good to me. ====== Kind Regards, Peter Smith. Fujitsu Australia
On Sun, Oct 13, 2024 at 11:23 PM Bertrand Drouvot
<bertranddrouvot.pg@gmail.com> wrote:
>
> Hi,
>
> On Mon, Oct 14, 2024 at 09:57:22AM +1100, Peter Smith wrote:
> > Here are some minor review comments for v15-0002.
> >
> > ======
> > contrib/pg_logicalinspect/pg_logicalinspect.c
> >
> > 1.
> > +pg_get_logical_snapshot_meta(PG_FUNCTION_ARGS)
> > +{
> > +#define PG_GET_LOGICAL_SNAPSHOT_META_COLS 3
> > + SnapBuildOnDisk ondisk;
> > + HeapTuple tuple;
> > + Datum values[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
> > + bool nulls[PG_GET_LOGICAL_SNAPSHOT_META_COLS] = {0};
> > + TupleDesc tupdesc;
> > + char path[MAXPGPATH];
> > + int i = 0;
> > + text *filename_t = PG_GETARG_TEXT_PP(0);
> > +
> > + sprintf(path, "%s/%s",
> > + PG_LOGICAL_SNAPSHOTS_DIR,
> > + text_to_cstring(filename_t));
> > +
> > + /* Build a tuple descriptor for our result type */
> > + if (get_call_result_type(fcinfo, NULL, &tupdesc) != TYPEFUNC_COMPOSITE)
> > + elog(ERROR, "return type must be a row type");
> > +
> > + /* Validate and restore the snapshot to 'ondisk' */
> > + SnapBuildRestoreSnapshot(&ondisk, path, CurrentMemoryContext, false);
> >
> > The sprintf should be deferred. Could you do it after the ERROR check?
>
> I think that makes sense, done in v16 attached.
>
> > ======
> > src/backend/replication/logical/snapbuild.c
> >
> > 3.
> > /*
> > - * Restore a snapshot into 'builder' if previously one has been stored at the
> > - * location indicated by 'lsn'. Returns true if successful, false otherwise.
> > + * Restore the logical snapshot file contents to 'ondisk'.
> > + *
> > + * If 'missing_ok' is true, will not throw an error if the file is not found.
> > + * 'context' is the memory context where the catalog modifying/committed xid
> > + * will live.
> > */
> > -static bool
> > -SnapBuildRestore(SnapBuild *builder, XLogRecPtr lsn)
> > +bool
> > +SnapBuildRestoreSnapshot(SnapBuildOnDisk *ondisk, const char *path,
> > + MemoryContext context, bool missing_ok)
> >
> > nit - I think it's better to describe parameters in the same order
> > that they are declared.
>
> Done in v16.
>
> > Also, include a 'path' description, so it is
> > not the only one omitted.
>
> I don't think that's worth it as self explanatory IMHO.
Thank you for updating the patches!
I fixed a compiler warning by -Wtypedef-redefinition related to the
declaration of SnapBuild struct, then pushed both patches.
Regards,
--
Masahiko Sawada
Amazon Web Services: https://aws.amazon.com