On Wed, Nov 27, 2024 at 12:45 PM vignesh C wrote:
>
> >
> > There is a buildfarm failure in [1]. One of the new tests added to
> > verify the log for the "incompatible generated columns" issue was
> > incorrect. Specifically, the check qr/ERROR: ( [A-Z0-9]:) should have
> > been updated to qr/ERRO
On Wed, 27 Nov 2024 at 12:15, vignesh C wrote:
>
> On Wed, 27 Nov 2024 at 08:50, Amit Kapila wrote:
> >
> > On Wed, Nov 27, 2024 at 3:30 AM Peter Smith wrote:
> > >
> > > Hi, here are some review comments for patch v7-0001.
> > >
> > > ==
> > > src/backend/replication/logical/relation.c
> >
On Wed, 27 Nov 2024 at 08:50, Amit Kapila wrote:
>
> On Wed, Nov 27, 2024 at 3:30 AM Peter Smith wrote:
> >
> > Hi, here are some review comments for patch v7-0001.
> >
> > ==
> > src/backend/replication/logical/relation.c
> >
> > logicalrep_report_missing_or_gen_attrs:
> >
> > 1.
> > +/*
> >
On Wed, Nov 27, 2024 at 3:30 AM Peter Smith wrote:
>
> Hi, here are some review comments for patch v7-0001.
>
> ==
> src/backend/replication/logical/relation.c
>
> logicalrep_report_missing_or_gen_attrs:
>
> 1.
> +/*
> + * If attempting to replicate missing or generated columns, report an erro
Hi, here are some review comments for patch v7-0001.
==
src/backend/replication/logical/relation.c
logicalrep_report_missing_or_gen_attrs:
1.
+/*
+ * If attempting to replicate missing or generated columns, report an error.
+ * Prioritize 'missing' errors if both occur though the prioritizat
On Tue, Nov 26, 2024 at 1:37 PM Shubham Khanna
wrote:
>
> I have fixed the given comments. The attached Patch contains the
> required changes.
>
The patch looks mostly good to me. I have made slight adjustments in
the comments and error message. The following proposed error message
appears to hav
On Tue, Nov 26, 2024 at 5:45 AM Peter Smith wrote:
>
> Hi Shubham,
>
> Here are my review comments for patch v5-0001.
>
> Please don't reply with a blanket "I have fixed the given comments"
> because it was not true. E.g., some of my previous comments are
> rejected in favour of Amit's better code
On Tue, Nov 26, 2024 at 9:47 AM Peter Smith wrote:
>
> On Tue, Nov 26, 2024 at 1:42 PM vignesh C wrote:
> >.
> >
> > Few comments:
> > 1) Now that attribute string generation is moved to get_attrs_str and
> > there are only a couple of error statements in this function, how
> > about removing the
On Tue, Nov 26, 2024 at 1:42 PM vignesh C wrote:
>.
>
> Few comments:
> 1) Now that attribute string generation is moved to get_attrs_str and
> there are only a couple of error statements in this function, how
> about removing the function:
> +/*
> + * If !bms_is_empty(missingatts), report the err
On Mon, 25 Nov 2024 at 16:06, Shubham Khanna
wrote:
>
> On Mon, Nov 25, 2024 at 8:50 AM Peter Smith wrote:
> >
> > Hi Shubham,
> >
> > here are my review comments for patch v4-0001.
> >
> > ==
> > src/backend/replication/logical/relation.c
> >
> > logicalrep_report_missing_and_gen_attrs:
> >
Hi Shubham,
Here are my review comments for patch v5-0001.
Please don't reply with a blanket "I have fixed the given comments"
because it was not true. E.g., some of my previous comments are
rejected in favour of Amit's better code suggestion, but then other
comments seem not addressed for reason
On Mon, Nov 25, 2024 at 5:27 PM Amit Kapila wrote:
>
> On Mon, Nov 25, 2024 at 8:50 AM Peter Smith wrote:
> >
> > 5.
> > As I reported above (#2), I think it is better to check for empty BMS
> > in the caller because then the code is easier to read. Also, you need
> > to comment on which of these
On Mon, Nov 25, 2024 at 8:50 AM Peter Smith wrote:
>
> Hi Shubham,
>
> here are my review comments for patch v4-0001.
>
> ==
> src/backend/replication/logical/relation.c
>
> logicalrep_report_missing_and_gen_attrs:
>
> 1.
> static void
> -logicalrep_report_missing_attrs(LogicalRepRelation *re
On Mon, Nov 25, 2024 at 8:50 AM Peter Smith wrote:
>
> 5.
> As I reported above (#2), I think it is better to check for empty BMS
> in the caller because then the code is easier to read. Also, you need
> to comment on which of these 2 errors will take precedence because if
> there are simultaneous
Hi Shubham,
here are my review comments for patch v4-0001.
==
src/backend/replication/logical/relation.c
logicalrep_report_missing_and_gen_attrs:
1.
static void
-logicalrep_report_missing_attrs(LogicalRepRelation *remoterel,
- Bitmapset *missingatts)
+logicalrep_report_missing_and_gen_attr
On Sat, Nov 16, 2024 at 5:43 PM Shlok Kyal wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> wrote:
> >
> > On Thu, Nov 14, 2024 at 2:09 PM Peter Smith wrote:
> > >
> > > Hi Shubham,
> > >
> > > +1 for the patch idea.
> > >
> > > Improving this error message for subscriber-side generated
On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
wrote:
>
> On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
>
> I have fixed the given comments. The attached Patch contains the
> required changes.
Couple of minor comments:
1) Since the previous error is going to exit, this pfree is not required:
+
On Mon, Nov 18, 2024 at 4:11 PM vignesh C wrote:
>
> On Mon, 18 Nov 2024 at 15:47, Shubham Khanna
> wrote:
> >
> > On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
> >
> > I have fixed the given comments. The attached Patch contains the
> > required changes.
>
> Couple of minor comments:
> 1) Si
On Fri, Nov 15, 2024 at 7:07 PM vignesh C wrote:
>
> On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
> wrote:
> >
> > I have fixed the given comments. The attached Patch contains the
> > required changes.
>
> Few comments:
> 1)
> a)You can mention that "If ismissing is true, report the error message
On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
wrote:
>
> On Thu, Nov 14, 2024 at 2:09 PM Peter Smith wrote:
> >
> > Hi Shubham,
> >
> > +1 for the patch idea.
> >
> > Improving this error message for subscriber-side generated columns
> > will help to remove some confusion.
> >
> > Here are my revi
On Fri, 15 Nov 2024 at 15:57, Shubham Khanna
wrote:
>
> I have fixed the given comments. The attached Patch contains the
> required changes.
Few comments:
1)
a)You can mention that "If ismissing is true, report the error message
as 'Missing replicated columns.' Otherwise, report the error message
On Thu, Nov 14, 2024 at 2:09 PM Peter Smith wrote:
>
> Hi Shubham,
>
> +1 for the patch idea.
>
> Improving this error message for subscriber-side generated columns
> will help to remove some confusion.
>
> Here are my review comments for patch v1-0001.
>
> ==
> Commit message.
>
> 1.
> The er
On Fri, Nov 15, 2024 at 8:19 AM Hayato Kuroda (Fujitsu)
wrote:
>
> Dear Shubham,
>
> Thanks for creating a patch! I checked yours and I have comments.
>
> 01.
> ```
> + StringInfoData gencolsattsbuf;
> + int generatedatts = 0;
> +
> + i
On Fri, Nov 15, 2024 at 9:06 AM Peter Smith wrote:
>
> On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila wrote:
> >
> > > A better solution may be just to *combine* everything, so the user
> > > only has to deal with one error. IIUC that's what is already happening
> > > in master code, so this patch d
On Fri, Nov 15, 2024 at 2:07 PM Amit Kapila wrote:
>
> On Fri, Nov 15, 2024 at 6:10 AM Peter Smith wrote:
> >
> > 3. A different approach?
> >
> > TBH, is introducing a whole new error message even a good idea?
> >
> > Now there are going to be two separate error messages where previously
> > the
On Fri, Nov 15, 2024 at 6:10 AM Peter Smith wrote:
>
> 3. A different approach?
>
> TBH, is introducing a whole new error message even a good idea?
>
> Now there are going to be two separate error messages where previously
> there was only one. So if the table has multiple problems at the same
> t
Dear Shubham,
Thanks for creating a patch! I checked yours and I have comments.
01.
```
+ StringInfoData gencolsattsbuf;
+ int generatedatts = 0;
+
+ initStringInfo(&gencolsattsbuf);
```
gencolsattsbuf is initialized at the beginning
Hi Shubham.
==
Commit message.
1.
FYI, to clarify my previous review comment [1] #1, I think a more
correct commit message might be:
SUGGESTION
Currently, if logical replication attempts to target a subscriber-side
table column that is either missing or generated, it produces the
following i
Hi Shubham,
+1 for the patch idea.
Improving this error message for subscriber-side generated columns
will help to remove some confusion.
Here are my review comments for patch v1-0001.
==
Commit message.
1.
The error message was misleading, as it failed to clarify that the replication
of r
Hi all,
Recently there was an issue reported by Kuroda-san on a different
thread [1]. I have created this thread to discuss the issue
separately.
Currently, the ERROR message for the replication of a regular column
on the publisher node to a generated column on the subscriber node
is:-
ERROR: log
30 matches
Mail list logo
