On Fri, Aug 16, 2024 at 2:47 PM vignesh C <vignes...@gmail.com> wrote:
>
> On Fri, 16 Aug 2024 at 10:04, Shubham Khanna
> <khannashubham1...@gmail.com> wrote:
> >
> > On Thu, Aug 8, 2024 at 12:43 PM Peter Smith <smithpb2...@gmail.com> wrote:
> > >
> > > Hi Shubham,
> > >
> > > I think the v25-0001 patch only half-fixes the problems reported in my
> > > v24-0001 review.
> > >
> > > ~
> > >
> > > Background (from the commit message):
> > > This commit enables support for the 'include_generated_columns' option
> > > in logical replication, allowing the transmission of generated column
> > > information and data alongside regular table changes.
> > >
> > > ~
> > >
> > > The broken TAP test scenario in question is replicating from a
> > > "not-generated" column to a "generated" column. As the generated
> > > column is not on the publishing side, IMO the
> > > 'include_generated_columns' option should have zero effect here.
> > >
> > > In other words, I expect this TAP test for 'include_generated_columns
> > > = true' case should also be failing, as I wrote already yesterday:
> > >
> > > +# FIXME
> > > +# Since there is no generated column on the publishing side this should
> > > give
> > > +# the same result as the previous test. -- e.g. something like:
> > > +# ERROR: logical replication target relation
> > > "public.tab_nogen_to_gen" is missing
> > > +# replicated column: "b"
> >
> > I have fixed the given comments. The attached v26-0001 Patch contains
> > the required changes.
>
> Few comments:
> 1) There's no need to pass include_generated_columns in this case; we
> can retrieve it from ctx->data instead:
> @@ -749,7 +764,7 @@ maybe_send_schema(LogicalDecodingContext *ctx,
> static void
> send_relation_and_attrs(Relation relation, TransactionId xid,
> LogicalDecodingContext *ctx,
> - Bitmapset *columns)
> + Bitmapset *columns,
> bool include_generated_columns)
> {
> TupleDesc desc = RelationGetDescr(relation);
> int i;
> @@ -766,7 +781,10 @@ send_relation_and_attrs(Relation relation,
> TransactionId xid,
>
> 2) Commit message:
> If the subscriber-side column is also a generated column then this option
> has no effect; the replicated data will be ignored and the subscriber
> column will be filled as normal with the subscriber-side computed or
> default data.
>
> An error will occur in this case, so the message should be updated
> accordingly.
>
> 3) The current test is structured as follows: a) Create all required
> tables b) Insert data into tables c) Create publications d) Create
> subscriptions e) Perform inserts and verify
> This approach can make reviewing and maintenance somewhat challenging.
>
> Instead, could you modify it to: a) Create the required table for a
> single test b) Insert data for this test c) Create the publication for
> this test d) Create the subscriptions for this test e) Perform inserts
> and verify f) Clean up
>
> 4) We can maintain the test as a separate 0002 patch, as it may need a
> few rounds of review and final adjustments. Once it's fully completed,
> we can merge it back in.
>
> 5) Once we create and drop publication/subscriptions for individual
> tests, we won't need such extensive configuration; we should be able
> to run them with default values:
> +$node_publisher->append_conf(
> + 'postgresql.conf',
> + "max_wal_senders = 20
> + max_replication_slots = 20");Fixed all the given comments. The attached patches contain the suggested changes. Thanks and Regards, Shubham Khanna.
v28-0002-Tap-tests-for-include-generated-columns.patch
Description: Binary data
v28-0001-Enable-support-for-include_generated_columns-opt.patch
Description: Binary data
