On Sun, Oct 13, 2024 at 11:23 PM Bertrand Drouvot
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_F
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
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
> + SnapBui
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
> wrote:
> >
> > On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot
> > wrote:
> > >
> > > Hi,
> > >
> > > On Thu, Oct 10, 2024 at 05:38:43PM -0700, Masahiko Sawada 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]
On Fri, Oct 11, 2024 at 11:15 AM Masahiko Sawada wrote:
>
> On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot
> 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
> > > wrote:
> > >
> > > The patches m
On Fri, Oct 11, 2024 at 6:15 AM Bertrand Drouvot
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
> > wrote:
> >
> > The patches mostly look good to me. Here are some minor comments:
>
> Thanks for looking at
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.
> ~~~
>
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
> wrote:
>
> The patches mostly look good to me. Here are some minor comments:
Thanks for looking at it!
>
> + sprintf(path, "%s/%s",
> + PG_LOGI
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 th
On Thu, Oct 10, 2024 at 6:10 AM Bertrand Drouvot
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
> > wrote:
> > > So I think that having
> > > construct_array_builtin()/deconstruct_array_builtin()
> > > takin
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
> 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
> > sub
On Wed, Oct 9, 2024 at 8:32 PM Bertrand Drouvot
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
> > wrote:
> > > One option could be (did not test it) to add this switch in
> > > construct_array_builtin():
>
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
> wrote:
> > One option could be (did not test it) to add this switch in
> > construct_array_builtin():
> >
> > + case XIDOID:
> > + elmlen =
On Wed, Oct 9, 2024 at 1:12 AM Bertrand Drouvot
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
> > wrote:
> >
> > Thank you for updating the patch! I have some comments on v12 patch:
>
> Thanks for looking at
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
> 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)
> + {
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
> 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 do
On Wed, Oct 9, 2024 at 3:25 AM Bertrand Drouvot
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| 136956
On Tue, Oct 8, 2024 at 9:25 AM Bertrand Drouvot
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.
> > ni
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
Hi, here are some review comments for patch v11.
==
contrib/pg_logicalinspect/specs/logical_inspect.spec
1.
nit - Add some missing spaces after commas (,) in the SQL.
nit - Also update the expected results accordingly
==
doc/src/sgml/pglogicalinspect.sgml
2.
+
+
+ The pg_logicalins
Hi,
On Wed, Sep 25, 2024 at 05:04:18PM -0700, Masahiko Sawada wrote:
> I've reviewed v10 patch and here are some comments:
Thanks for looking at it!
>
> +static void
> +ValidateAndRestoreSnapshotFile(XLogRecPtr lsn, SnapBuildOnDisk *ondisk,
> + const char *path)
On Wed, Sep 25, 2024 at 11:01 PM Bertrand Drouvot
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 an
Hi,
On Wed, Sep 25, 2024 at 10:29 AM Bertrand Drouvot
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);
> > +
>
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,
> +
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 typ
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)
+
On Tue, Sep 24, 2024 at 10:23 PM Bertrand Drouvot
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
> > wrote:
> > >
> > >
> > > Please find attached v8, that:
> > >
> >
> > Thank You for the patch. In one of my
On Wed, Sep 25, 2024 at 2:51 AM Bertrand Drouvot
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.
> > + */
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
> 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 p
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 SnapBuildS
On Mon, Sep 23, 2024 at 7:57 AM 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[] = {
On Fri, Sep 20, 2024 at 12:22 PM Bertrand Drouvot
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
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_S
Hi,
On Thu, Sep 19, 2024 at 10:08:19AM +1000, Peter Smith wrote:
> Thanks for the updated patch.
>
> ==
> .../expected/logical_inspect.out
>
> 2
> +step s1_get_logical_snapshot_info: SELECT
> (pg_get_logical_snapshot_info(f.name::pg_lsn)).state,(pg_get_logical_snapshot_info(f.name::pg_lsn)).
On Thu, Sep 19, 2024 at 12:17 AM Bertrand Drouvot
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 s
On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot
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_lo
Thanks for the updated patch.
Here are a few more trivial comments for the patch v7-0001.
==
1.
Should the extension descriptions all be identical?
I noticed small variations:
contrib/pg_logicalinspect/Makefile
+PGFILEDESC = "pg_logicalinspect - functions to inspect logical
decoding compon
Hi,
On Wed, Sep 18, 2024 at 07:52:51PM +1000, Peter Smith wrote:
> HI, here are some mostly minor review comments for the patch v5-0001.
>
Thanks for the review!
> ==
> Commit message
>
> 1.
> Do you think you should also name the new functions here?
Not sure about this one. It has not be
HI, here are some mostly minor review comments for the patch v5-0001.
==
Commit message
1.
Do you think you should also name the new functions here?
==
contrib/pg_logicalinspect/pg_logicalinspect.c
2.
Regarding the question about static function declarations:
Shveta wrote: I was someho
Hi,
On Wed, Sep 18, 2024 at 11:33:08AM +0530, shveta malik wrote:
> Thanks for the patch. Few trivial things:
>
> 1)
> May be we shall change 'INTERNAL_SNAPBUILD_H' in snapbuild_internal.h
> to 'SNAPBUILD_INTERNAL_H'?
Indeed, done in v6 attached, thanks!
> 2)
> ValidateSnapshotFile()
>
> It is
On Tue, Sep 17, 2024 at 12:44 PM Bertrand Drouvot
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"
> >
On Tue, Sep 17, 2024 at 10:46 AM David G. Johnston
wrote:
>
>
>
> On Monday, September 16, 2024, shveta malik wrote:
>>
>> On Tue, Sep 17, 2024 at 10:18 AM shveta malik wrote:
>> >
>> > Thanks for addressing the comments. I have not started reviewing v4
>> > yet, but here are few more comments o
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
> in
Hi,
On Mon, Sep 16, 2024 at 10:16:16PM -0700, David G. Johnston wrote:
> On Monday, September 16, 2024, shveta malik wrote:
>
> > On Tue, Sep 17, 2024 at 10:18 AM shveta malik
> > wrote:
> > >
> > > Thanks for addressing the comments. I have not started reviewing v4
> > > yet, but here are few
On Monday, September 16, 2024, shveta malik wrote:
> On Tue, Sep 17, 2024 at 10:18 AM shveta malik
> 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
On Tue, Sep 17, 2024 at 10:18 AM shveta malik 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
On Mon, Sep 16, 2024 at 8:03 PM Bertrand Drouvot
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
> > wrote:
> > >
> > >
> > > Yeah, good idea. Done that way in v3 attached.
> > >
> >
> > Thanks for the patch. +1
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
> 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 plea
On Wed, Sep 11, 2024 at 4:21 PM Bertrand Drouvot
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)
+ fsy
Hi,
On Wed, Sep 11, 2024 at 10:30:37AM +0530, Amit Kapila wrote:
> On Tue, Sep 10, 2024 at 8:56 PM Bertrand Drouvot
> 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
> > > wrote:
> > > > as we decided not to expos
On Tue, Sep 10, 2024 at 8:56 PM Bertrand Drouvot
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
> > wrote:
> > > as we decided not to expose the SnapBuildOnDisk and SnapBuild structs to
> > > public
> > > and to create
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
> 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
> >
On Fri, Aug 30, 2024 at 5:18 PM Bertrand Drouvot
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
> > wrote:
> > >
> > > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot
> > > wrote:
> > > >
> > > > Yeah that's fair. An
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
> wrote:
> >
> > On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot
> > wrote:
> > >
> > > Yeah that's fair. And now I'm wondering if we need an extra module. I
> > > think
> > > w
On Thu, Aug 29, 2024 at 6:33 PM Bharath Rupireddy
wrote:
>
> On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot
> 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?
Hi,
On Thu, Aug 29, 2024 at 02:15:47PM +, Bertrand Drouvot wrote:
> 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).
>
Please find v2 attached that creates the 2 ne
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
> 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 fo
On Thu, Aug 29, 2024 at 3:44 PM Bertrand Drouvot
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 c
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
> 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
> > > wrote:
> > > >
> > > > 2. The SnapBuildOn
On Wed, Aug 28, 2024 at 1:25 AM Bertrand Drouvot
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
> > wrote:
> > >
> > > 2. The SnapBuildOnDisk and SnapBuild structs are now exposed to public.
> > > Means
> > > we should
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
> wrote:
> >
> > Please find attached a patch to $SUBJECT.
> >
> > This module provides SQL functions to inspect the contents of serialized
> > logical
> > snapshots of a running d
On Thu, Aug 22, 2024 at 5:56 PM Bertrand Drouvot
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 purp
pen Source Databases
Amazon Web Services: https://aws.amazon.com
>From cc57b8cda2ec7d7bcd5122f20f2b2c9950840998 Mon Sep 17 00:00:00 2001
From: Bertrand Drouvot
Date: Wed, 14 Aug 2024 08:46:05 +
Subject: [PATCH v1] Add contrib/pg_logicalsnapinspect
Provides SQL functions that allow to inspect the contents of serial
64 matches
Mail list logo
