Hi, On Thu, Jan 25, 2024 at 03:54:45PM +0530, Amit Kapila wrote: > On Thu, Jan 25, 2024 at 1:25 PM Bertrand Drouvot > <bertranddrouvot...@gmail.com> wrote: > > > > On Thu, Jan 25, 2024 at 02:57:30AM +0000, Zhijie Hou (Fujitsu) wrote: > > > > > > Agreed. I split the original 0001 patch into 3 patches as suggested. > > > Here is the V68 patch set. > > Thanks, I have pushed 0001. > > > > > Thanks! > > > > Some comments. > > > > Looking at 0002: > > > > 1 === > > > > + <para>The following options are supported:</para> > > > > What about "The following option is supported"? (as currently only the > > "FAILOVER" > > is) > > > > 2 === > > > > What about adding some TAP tests too? (I can see that > > ALTER_REPLICATION_SLOT test > > is added in v68-0004 but I think having some in 0002 would make sense too). > > > > The subscription tests in v68-0003 will test this functionality. The > one advantage of adding separate tests for this is that if in the > future we extend this replication command, it could be convenient to > test various options. However, the same could be said about existing > replication commands as well.
I initially did check for "START_REPLICATION" and I saw it's part of 006_logical_decoding.pl (but did not check all the "REPLICATION" commands). That said, it's more a Nit and I think it's fine with having the test in v68-0004 (as it is currently done) + the ones in v68-0003. > But is it worth having extra tests which > will be anyway covered in the next commit in a few days? > > I understand that it is a good idea and makes one comfortable to have > tests for each separate commit but OTOH, in the longer term it will > just be adding more test time without achieving much benefit. I think > we can tell explicitly in the commit message of this patch that the > subsequent commit will cover the tests for this functionality Yeah, I think that's enough (at least someone reading the commit message, the diff changes and not following this dedicated thread closely would know the lack of test is not a miss). > One minor comment on 0002: > + so that logical replication can be resumed after failover. > + </para> > > Can we move this and similar comments or doc changes to the later 0004 > patch where we are syncing the slots? Sure. Regards, -- Bertrand Drouvot PostgreSQL Contributors Team RDS Open Source Databases Amazon Web Services: https://aws.amazon.com
