gs. To
customize other aspects of the standby setup, a new option
standby_config can be supported in the future if required.
[1] -
https://www.postgresql.org/message-id/20220221232849.x6s24ete4eyg6jol%40alap3.anarazel.de
Thanks and regards,
Shubham Khanna.
v1-0001-pg_basebackup-via-pg_createsubscriber.patch
Description: Binary data
On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
wrote:
>
> On Monday, March 17, 2025, Shubham Khanna wrote:
>>
>>
>> I have added validation for "all" to address both issues at once.
>>
>
> Usage needs to be changed to refer to object types an
On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
wrote:
>
> On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> wrote:
>
> >
> > The attached patches contain the suggested changes.
> >
>
> I have started reviewing the patches again. Here are some review
On Thu, Mar 20, 2025 at 5:54 PM Amit Kapila wrote:
>
> On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> wrote:
> >
> > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
> > wrote:
> >
> >
> > +# run pg_createsubscriber with '--database'
On Thu, Mar 27, 2025 at 12:16 PM Amit Kapila wrote:
>
> On Wed, Mar 26, 2025 at 4:02 PM Shubham Khanna
> wrote:
> >
>
> Let's combine 0001 and 0002.
>
Combined 0001 and 0002.
> A few minor comments:
> *
> + If database name is not specifi
On Wed, Mar 26, 2025 at 10:21 AM Amit Kapila wrote:
>
> On Wed, Mar 26, 2025 at 2:05 AM Euler Taveira wrote:
> >
> > On Tue, Mar 25, 2025, at 8:07 AM, Shubham Khanna wrote:
> >
> > Apologies for the oversight. I have attached the patches now. Please
> > fi
On Tue, Mar 25, 2025 at 4:31 PM Ashutosh Bapat
wrote:
>
> On Tue, Mar 25, 2025 at 4:28 PM Shubham Khanna
> wrote:
>
> >
> > The attached patches contain the suggested changes.
> > [1] -
> > https://www.postgresql.org/message-id/CAA4eK1KUDEO0t6
se particular checks
were retained in the main patch because they ensure the fundamental
validations for incompatible options (--database and --publication
with --all) are covered early on.
> 03.
> ```
> +# Verify that the required logical replication objects are created. The
> +# expected count 3 refers to postgres, $db1 and $db2 databases.
> ```
>
> Hmm, but since we did a dry-run, any objects are not created, right?
>
Fixed.
The attached patches contain the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAA4eK1KUDEO0t6i16_CcEpg33sgcgEddHcdVC_q8j4tVUb5FWw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Mon, Mar 24, 2025 at 6:08 PM Ashutosh Bapat
wrote:
>
> On Fri, Mar 21, 2025 at 6:59 PM Shubham Khanna
> wrote:
> >
> > On Thu, Mar 20, 2025 at 4:53 PM Ashutosh Bapat
> > wrote:
> > >
> > > On Thu, Mar 20, 2025 at 10:25 AM Shubham Khanna
>
On Mon, Mar 24, 2025 at 5:41 PM Shlok Kyal wrote:
>
> On Mon, 24 Mar 2025 at 15:56, Shubham Khanna
> wrote:
> >
> > On Mon, Mar 24, 2025 at 12:29 PM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > > > I
wait_for_subscription_sync
> was used there, but I feel this may not be correct). How about unifing them
> like
> attached?
>
Fixed.
> 08.
> ```
> +# Verify logical replication works for all databases
> +# Insert rows on node P
> +$node_p->safe_psql('pos
On Sat, Mar 22, 2025 at 6:23 PM vignesh C wrote:
>
> On Fri, 21 Mar 2025 at 18:59, Shubham Khanna
> wrote:
> >
> >
> > During the recent testing, I observed that the tests were failing when
> > using wait_for_slot_catchup(). To address this, I reverted to usi
On Thu, Mar 20, 2025 at 3:41 PM vignesh C wrote:
>
> On Thu, 20 Mar 2025 at 10:25, Shubham Khanna
> wrote:
> >
> >
> > I have created two patches, v16-0001 and v16-0002, to address the
> > performance issue. I conducted performance testing, and here are the
>
On Wed, Mar 19, 2025 at 2:29 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 11:44, Shubham Khanna
> wrote:
> >
> >
> > I agree with you on this; switching to a single query with safe_psql()
> > will indeed simplify the test and make the verification logic cl
On Thu, Mar 20, 2025 at 9:37 AM vignesh C wrote:
>
> On Thu, 20 Mar 2025 at 09:06, Shubham Khanna
> wrote:
> >
> > The attached patch contains the suggested changes.
>
> Couple of minor comments:
> 1) The second and third line comments can be in same line:
> +#
On Wed, Mar 19, 2025 at 3:15 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 14:32, Shubham Khanna
> wrote:
> >
> >
> > Changed -r to -R based on the shared analysis to avoid conflict with
> > the --retain option used in pg_upgrade and to maintain consiste
/60b45b8a-3047-4a21-ba2a-ddb15daa638f%40eisentraut.org
> [2]: https://www.postgresql.org/docs/devel/pgupgrade.html
>
Changed -r to -R based on the shared analysis to avoid conflict with
the --retain option used in pg_upgrade and to maintain consistency
across tools.
The attached patch contains the
) similar to
> pg_upgrade. So, using -R for this option sounds reasonable to me.
>
> --
Fixed the mentioned issue by changing -r to -R to avoid conflict with
the --retain option used in pg_upgrade. The attached patch at [1]
contains the required change.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKbYaCRrzJzmU6gU0jt85xJftzVV-Hu9rXrkb1yFzYTtA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Mar 19, 2025 at 12:44 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Wed, Mar 19, 2025 at 2:55 PM Shubham Khanna wrote:
> >
> > I have merged the changes and prepared the latest patch. The attached
> > patch contains the suggested changes.
>
> Thanks for updating th
On Wed, Mar 19, 2025 at 12:09 PM vignesh C wrote:
>
> On Wed, 19 Mar 2025 at 10:39, Shubham Khanna
> wrote:
> >
> > On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila wrote:
> > >
> > > On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> > > wrote:
> &
On Wed, Mar 19, 2025 at 11:17 AM vignesh C wrote:
>
> On Tue, 18 Mar 2025 at 17:34, Shubham Khanna
> wrote:
> >
> > I have added an additional test case to 040_pg_createsubscriber.pl as
> > suggested.
> >
> > The attached patch contains the suggested cha
as been removed
to align with the statement that they are redundant and serve no
purpose.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJNE1ZvWhsgL54iPsJhomhcG%2B-SGPN8AnnwdLmWt6A44A%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
to affect or stop the dropping
> of the selected objects so consider taking a backup of them using pg_dump.
>
>
> Just add more paragraphs next to "publications:" as we add more object types.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJNE1ZvWhsgL54iPsJhomhcG%2B-SGPN8AnnwdLmWt6A44A%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Tue, Mar 18, 2025 at 5:17 PM Amit Kapila wrote:
>
> On Tue, Mar 18, 2025 at 4:01 PM Shubham Khanna
> wrote:
> >
> > On Tue, Mar 18, 2025 at 12:07 PM David G. Johnston
> > wrote:
> > >
> >
> > > It would be good if we could get this to play n
On Tue, Mar 18, 2025 at 4:25 PM vignesh C wrote:
>
> On Mon, 17 Mar 2025 at 16:51, Ashutosh Bapat
> wrote:
> >
> > On Mon, Mar 17, 2025 at 4:02 PM vignesh C wrote:
> > >
> > > On Mon, 17 Mar 2025 at 11:28, Shubham Khanna
> > > wrote:
> > &
On Tue, Mar 18, 2025 at 5:48 AM David G. Johnston
wrote:
>
> On Mon, Mar 17, 2025 at 4:01 PM Euler Taveira wrote:
>>
>> On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
>>
>> I have incorporated the "--remove/-r" parameter in the attached
On Tue, Mar 18, 2025 at 4:31 AM Euler Taveira wrote:
>
> On Mon, Mar 17, 2025, at 9:34 AM, Shubham Khanna wrote:
>
> I have incorporated the "--remove/-r" parameter in the attached patch,
> as it seems more intuitive and straightforward for users.
> The attached patch
s, so we must
> communicate it via the description.
>
> --
I have incorporated the "--remove/-r" parameter in the attached patch,
as it seems more intuitive and straightforward for users.
The attached patch contains the latest changes.
Thanks and regards,
Shubham Khanna,
v17-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
ase refer to the attached top-up fix patch, which includes the
> above changes along with some cosmetic fixes in the test file
> 042_all_option.pl.
>
> [1]
> https://www.postgresql.org/message-id/CAExHW5uJHYAge99oS_iPfGWwZ_eCr2xFCNnifQkGs2GXeMQKGQ%40mail.gmail.com
>
> --
Fixed.
On Mon, Mar 10, 2025 at 5:02 PM Nisha Moond wrote:
>
> On Mon, Mar 10, 2025 at 4:31 PM Shubham Khanna
> wrote:
> >
> > > 2) This part of code has another bug:
> > > "dbinfos.dbinfo->made_publication = false;" incorrectly modifies
> > > d
On Mon, Mar 10, 2025 at 3:09 PM Nisha Moond wrote:
>
> On Mon, Mar 10, 2025 at 12:11 PM Shubham Khanna
> wrote:
> >
> > On Thu, Mar 6, 2025 at 9:27 AM Peter Smith wrote:
> > >
> > > 6.
> > > - dbinfo->pubname, dbinfo->dbname, PQresultErrorM
checked with grammarly as well.
>
Fixed.
The attached patch at [1] contains the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2B37ja589BzqB5bz0ZYGWb5gpnP9of8SoqKc%3DDqLmvxBg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
uggesting this change?
>
> --
Updated the .sgml file accordingly. The attached patch at [1] contains
the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2B37ja589BzqB5bz0ZYGWb5gpnP9of8SoqKc%3DDqLmvxBg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Thu, Mar 6, 2025 at 9:18 AM Ajin Cherian wrote:
>
> On Fri, Feb 28, 2025 at 11:34 PM Shubham Khanna
> wrote:
> >
> >
> > The attached Patch contains the suggested changes.
> >
> > Thanks and regards,
> > Shubham Khanna.
>
> S
tch
it directly from the instance for comparison, making the loop more
meaningful.
Additionally, as suggested by Ashutosh in [1], I have split the
040_pg_createsubscriber.pl file into three parts to improve clarity.
The attached patch includes all the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAExHW5uJHYAge99oS_iPfGWwZ_eCr2xFCNnifQkGs2GXeMQKGQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
subscriber on node S using '--cleanup-existing-publications'.
> +# --verbose is used twice to show more information.
> # In passing, also test the --enable-two-phase option
> command_ok(
> [
> 'pg_createsubscriber',
> '--verbose', '--verbose',
> '--recovery-timeout' => $PostgreSQL::Test::Utils::timeout_default,
> - '--pgdata' => $node_s->data_dir,
> + '--pgdata' => $node_s1->data_dir,
> '--publisher-server' => $node_p->connstr($db1),
> - '--socketdir' => $node_s->host,
> - '--subscriber-port' => $node_s->port,
> + '--socketdir' => $node_s1->host,
> + '--subscriber-port' => $node_s1->port,
> '--publication' => 'pub1',
> '--publication' => 'pub2',
> '--replication-slot' => 'replslot1',
> '--replication-slot' => 'replslot2',
> '--database' => $db1,
> '--database' => $db2,
> - '--enable-two-phase'
> + '--enable-two-phase',
> + '--cleanup-existing-publications',
> ],
> 'run pg_createsubscriber on node S');
>
> Comment and Message should refer to S1, not S.
>
> ~~~
>
Fixed.
> 15.
> +# Create user-defined publications
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub3 FOR ALL TABLES;");
> +$node_p->safe_psql($db3, "CREATE PUBLICATION test_pub4 FOR ALL TABLES;");
>
> Probably these can be combined.
>
> ~~~
>
Fixed.
> 16.
> +# Drop the newly created publications
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub3;");
> +$node_p->safe_psql($db3, "DROP PUBLICATION IF EXISTS test_pub4;");
> +
>
> Probably these can be combined.
>
Fixed.
The attached patch at [1] contains the required changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjLCZyzFMGR8SBdvSocRZGAAr_eRd4ht48kXCp9oEaKQeQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Mar 5, 2025 at 3:55 PM Nisha Moond wrote:
>
> On Tue, Mar 4, 2025 at 8:05 PM Shubham Khanna
> wrote:
> >
> > The attached Patch contains the suggested changes.
> >
>
> Hi Shubham,
>
> Here are few comments for 040_pg_createsubscriber.pl
>
> 1)
On Tue, Mar 4, 2025 at 4:31 PM Nisha Moond wrote:
>
> On Tue, Mar 4, 2025 at 10:30 AM Shubham Khanna
> wrote:
> >
> > Fixed the suggested changes. The attached patch contains the required
> > changes.
> >
>
> Hi Shubham,
>
> Thanks for the patch! I
lease do not modify randomly. Currently, made_publication is set to false
> when
> the command fails. No need to set to true here - Please follow that. Also,
> please
> do not remove comments within the if statement.
>
Fixed the suggested changes. The attached patch contains the
t; > ```
> > + -a
> > + --all-dbs
> > ```
> >
> > Peter's comment [2] does not say that option name should be changed.
> > The scope of his comment is only in the .c file.
>
> Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
> referring only to the field name of struct CreateSubscriberOptions,
> nothing else. Not the usage help, not the error messages, not the
> docs, not the tests, not the commit message.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ocess.
As of the latest patch attached at [1], this issue remains unresolved.
I will proceed with splitting the tests once all other issues are
addressed.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
below.
> > >
> > > 01.
> > > ```
> > > + -a
> > > + --all-dbs
> > > ```
> > >
> > > Peter's comment [2] does not say that option name should be changed.
> > > The scope of his comment is only in the .c file.
> >
> > Yes, that's correct. My v9 suggestion to change 'all' to 'all_dbs' was
> > referring only to the field name of struct CreateSubscriberOptions,
> > nothing else. Not the usage help, not the error messages, not the
> > docs, not the tests, not the commit message.
>
> +1. We don't want yet another option to specify all databases. :)
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJ9BsCg%2Bpur307b1JnfQebnmxFZLw4LdcGX7f-%3D6OK1vw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
;
> foreach my $dbname (@user_dbs)
> {
> $result =
> $node_s2->safe_psql('postgres',
> "SELECT count(*) FROM pg_subscription, pg_database
> WHERE subdbid = pg_database.oid and datname = '$dbname';");
>
&g
ovacuum = off');
> +$node_a->start;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
> [1]:
> https://www.postgresql.org/message-id/OSCPR01MB14966D845AC77FC46ECEECCE4F5C72%40OSCPR01MB14966.jpnprd01.prod.outlook.com
>
The v10 patch at [1] required rebasing on the latest HEAD. I have
prepared a rebased version and updated the patch accordingly.
[1] -
https://www.postgresql.org/message-id/CAHv8RjJtzXc4BWoKTyTB-9WEcAJ4N5qsD6YuXK3EAmsT6237yw%40mail.gmail.com
The attached patch includes the suggested changes.
Thanks and regards,
Shubham Khanna.
v11-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
subscriber.pl
> ```
> +# Set up node A as primary
> +my $node_a = PostgreSQL::Test::Cluster->new('node_a');
> +my $aconnstr = $node_a->connstr;
> +$node_a->init(allows_streaming => 'logical');
> +$node_a->append_conf('postgresql.conf', 'autovacuum = off');
> +$node_a->start;
> ```
>
> I don't think new primary is not needed. Can't you reuse node_p?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Support-for-dropping-all-publications-in-pg_crea.patch
Description: Binary data
g_log_info("dropping publication \"%s\" in database \"%s\"",
> dbinfo->pubname, dbinfo->dbname);
> pg_log_debug("command is: %s", str->data);
> ```
>
Fixed.
> 08.
> I feel we must update made_publication.
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BzmkwunNubo%2B_Gp26S_qXbD%3Dp%2BrMfEnPnjiEE1A6GTXQ%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Fri, Feb 21, 2025 at 8:31 AM Peter Smith wrote:
>
> On Thu, Feb 20, 2025 at 5:44 PM Shubham Khanna
> wrote:
> >
> > On Mon, Feb 17, 2025 at 9:49 AM Peter Smith wrote:
> > >
> ...
> > > ==
> > > 1 GENERAL. Option Name?
> > >
&
On Mon, Feb 24, 2025 at 10:57 AM Amit Kapila wrote:
>
> On Tue, Feb 18, 2025 at 12:16 AM Shubham Khanna
> wrote:
> >
>
> -static void check_publisher(const struct LogicalRepInfo *dbinfo);
> -static char *setup_publisher(struct LogicalRepInfo *dbinfo);
> +static void c
will be 3, 3, 3. Why?
>
> I guess you intended to verify that relevant subscriptions were
> created for each of the target databases. But this code is not doing
> that.
>
> ~~~
Fixed.
>
> 7.
> +# Verify replication is working
> +$node_p->safe_psql($db1, "INSER
> +
> +$result = $node_s1->safe_psql($db1, "SELECT * FROM tbl1 ORDER BY 1");
> +is( $result, qq(first row
> +replication test
> +second row), "replication successful in $db1");
> +
> +$node_s1->stop;
> # Run pg_createsubscriber on node S. --verbose is used twice
> # to show more information.
> command_ok(
> ```
>
> I'm not sure the part is not needed. This have already been checked in other
> parts, right?
>
Fixed.
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Enhance-pg_createsubscriber-to-fetch-and-append-.patch
Description: Binary data
EATE PUBLICATION test_pub FOR ALL TABLES;");
> +$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
> +
>
> /it's/their/
>
> ~~~
>
Fixed.
> 8.
> Maybe also do a CREATE PUBLICATION at node_s, prior to the
> pg_createsubvscript, so then you can verify that the user-created one
> is unaffected by the cleanup of all the others.
>
> ==
Since $node_s is a streaming standby, it does not allow object
creation. As a result, publications cannot be created on $node_s.
> [1]
> https://www.postgresql.org/message-id/CAExHW5t4ew7ZrgcDdTv7YmuG7LVQT1ZaEny_EvtngHtEBNyjcQ%40mail.gmail.com
>
The attached patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v8-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
On Tue, Feb 18, 2025 at 2:43 PM Zhijie Hou (Fujitsu)
wrote:
>
> On Thursday, February 13, 2025 11:28 AM Shubham Khanna
> wrote:
>
> Hi,
>
> > The attached patch contains the required changes.
>
> Thanks for updating the patch. I think it's a useful featur
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKAdrrt3-pF6yHb5gBricB9%3DD7O47Dxe39zRxKkShdpmw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Fri, Feb 14, 2025 at 5:27 PM Ashutosh Bapat
wrote:
>
> Hi Shubham,
> Here are some review comments from my side
>
>
> On Thu, Feb 13, 2025 at 8:58 AM Shubham Khanna
> wrote:
> > The attached patch contains the required changes.
> >
>
> clusterdb, vacuumd
On Mon, Feb 17, 2025 at 9:43 PM vignesh C wrote:
>
> On Wed, 22 Jan 2025 at 16:23, Shubham Khanna
> wrote:
> >
> > Hi,
> >
> > Following the recent comments on Patch v13-0001, the patch was not
> > applied to the HEAD. I have addressed the feedbac
On Mon, Feb 17, 2025 at 4:06 PM Amit Kapila wrote:
>
> On Thu, Jan 23, 2025 at 6:42 PM Shubham Khanna
> wrote:
> >
> > Fixed the given comments. The attached patch contains the suggested changes.
> >
>
> I am fine with giving a WARNING for max_slot_wal_keep_s
On Fri, Feb 14, 2025 at 4:57 PM Shlok Kyal wrote:
>
> On Fri, 14 Feb 2025 at 10:50, Shubham Khanna
> wrote:
> >
> > On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal wrote:
> > >
> > > On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
> > > wrote:
> >
On Thu, Feb 13, 2025 at 5:36 PM Shlok Kyal wrote:
>
> On Thu, 13 Feb 2025 at 15:20, Shubham Khanna
> wrote:
> >
> > On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal wrote:
> > >
> > > On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
> > > wrote:
>
> patch seems not to do. Below example shows the case when three publications
> exist.
>
> ```
> pg_createsubscriber: dropping publications "aaa, bbb, ccc" in database
> "postgres"
> ```
>
> I think the output must be `"aaa", "bbb", "ccc"`. This means
> drop_publication() should be refactored.
>
> [1]:
> https://www.postgresql.org/docs/devel/error-style-guide.html#ERROR-STYLE-GUIDE-QUOTES
>
Fixed.
The attached patch at [1] contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKGywu%2B3cAv9MPgxi1_TUXBT8yzUsW%2Bf%3Dg5UsgeJ8Uk6g%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal wrote:
>
> On Tue, 11 Feb 2025 at 09:51, Shubham Khanna
> wrote:
> >
> > On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > > Thanks for u
them and adding publications.
>
> IMO a better choice of adjectives can be found below.
> "no suitable databases found..."
> "no appropriate databases found..."
> "no eligible databases found..."
> etc.
>
Fixed.
The attached patch contains the required changes.
Thanks and regards,
Shubham Khanna.
v8-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
On Wed, Feb 12, 2025 at 1:15 PM Shubham Khanna
wrote:
>
> On Wed, Feb 12, 2025 at 5:29 AM Peter Smith wrote:
> >
> > On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> > wrote:
> > >
> > > > #13. Unanswered question "How are tests expecting th
pg_log_error_hint("Ensure that there are non-template and
> connectable databases on the source server.");
> ```
>
Fixed the given comment. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8RjKc6LMJ86b4yCmwNTn0c65mz0BGMCF-vPJSKDMOGagVGA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Feb 12, 2025 at 5:29 AM Peter Smith wrote:
>
> On Tue, Feb 11, 2025 at 9:16 PM Shubham Khanna
> wrote:
> >
> > > #13. Unanswered question "How are tests expecting this even passing?".
> > > Was a reason identified? IOW, how can we be sure
>
> (same as the previous review comment).
>
> Previously reported. Claimed fixed -a to --all-databases. Not fixed.
>
Fixed.
> ==
> src/bin/pg_basebackup/pg_createsubscriber.c
>
> 5.
> + bool all_databases; /* fetch and specify all databases */
>
> Comment wording. "and specified" (??). Maybe just "--all-databases
> option was specified"
>
> ==
Fixed.
The attached Patch contains the suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
v6-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
ated before --cleanup-publisher-objects is run');
> +
> ```
>
> Also, there is a possibility that CREATE PUBLICATION on node_p is not
> replicated yet
> when SELECT COUNT(*) is executed. Please wait for the replay.
>
> [1]:
> https://www.postgresql.org/message-id/CAHv8RjL4OvoYafofTb_U_JD5HuyoNowBoGpMfnEbhDSENA74Kg%40mail.gmail.com
> [2]:
> https://www.postgresql.org/message-id/OSCPR01MB1496664FDC38DA40A441F449FF5EE2%40OSCPR01MB14966.jpnprd01.prod.outlook.com
> [3]: https://www.postgresql.org/docs/devel/source-format.html
>
Fixed.
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v4-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
tabase cannot be
> used with --all-databases");
> + exit(1);
> +
> ```
>
> I think this erroring is not enough. getopt_long() receives options with the
> specified ordering, thus
> -d can be accepted if the -a is specified latter.
&
"--replslot"
>
> ~~~
>
> 12.
> - * If --database option is not provided, try to obtain the dbname from
> - * the publisher conninfo. If dbname parameter is not available, error
> - * out.
> + * If neither --database nor --all-databases option is provided, try
> + * to obtain the dbname from the publisher conninfo. If dbname
> + * parameter is not available, error out.
>
> For this comment to make sense I think the previous comment (where
> fetch_source_databases is called) needs to explain that when
> --all-databases is defined then it is going to treat that internally
> as the equivalent of a whole lot of user-specified --database options.
>
> ==
> .../t/040_pg_createsubscriber.pl
>
> 13.
> + qr/cannot specify --publication or --replication-slot when using
> --all-databases/,
> + 'fail if --all-databases is used with publication and slot');
> +
>
> How are tests expecting this even passing?
>
> Searching the patch I cannot find any such error!
>
> ==
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BmhYgh8XLFM8sN8J05z0ia%2BknfB1kP6kjbnB55H-b-mw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
> 16a.
> Why is this code checking 'opt->all_databases'? Isn't it only possible
> to get to this function via --all-databases. You can just Assert this.
>
> ~
>
> 16b.
> Why are you reusing 'slot_name' variable like this?
>
> ~
>
> 16c.
> Was there some ERROR checking missing here to ensure the length of the
> dbname is not going to cause pub/slot to exceed NAMEDATALEN?
>
> ~
>
> 16d.
> In hindsight, most of this function seems unnecessary to me. Probably
> you could've done all this pub/slot name assignment inside the
> fetch_all_databases() at the same time as you were doing
> simple_string_list_append(&opt->database_names, dbname); for each
> database.
>
> ~~~
>
> 17.
> - while ((c = getopt_long(argc, argv, "d:D:np:P:s:t:U:v",
> + while ((c = getopt_long(argc, argv, "ad:D:np:P:s:t:U:v",
> long_options, &option_index)) != -1)
>
>
> Isn't the code within this loop missing all the early switch
> validation for checking you are not combining incompatible switches?
>
> e.g.
> --all-databases - "Cannot specify --all-databases more than once"
> --all-databases - "Cannot combine with --database, --publication,
> --subscription, --replication-slot"
> --database - "Cannot combine with --all-databases"
> --publication - "Cannot combine with --all-databases"
> --subscription - "Cannot combine with --all-databases"
> --replication-slot - "Cannot combine with --all-databases"
>
> ~~~
>
> 18.
> +
> +
> /* Number of object names must match number of databases */
>
> Remove spurious whitespace.
>
> ==
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
;
> +is($db_count_before, '1', 'database count without --all-databases option');
> ```
>
> This test is not actually realted with our codes, no need to do.
>
> 11. 040_pg_createsubscriber.pl
> ```
> +# Ensure there are some user databases on the publisher
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db3');
> +$node_a->safe_psql('postgres', 'CREATE DATABASE db4');
> +
> +$node_b->stop;
> ```
>
> You must wait until all changes have been replicated to the standby here.
>
> 12. 040_pg_createsubscriber.pl
>
> Can we do the similar tests without adding new instances? E.g., runs with
> dry-run mode and confirms all databases become target.
>
Fixed the given comments. The attached patch at [1] contains the
suggested changes.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BDOQ9vvkKCsmDeFKyM073yRfs47Tn_qywQOdx15pmOMA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Wed, Feb 5, 2025 at 5:31 PM Shlok Kyal wrote:
>
> On Wed, 5 Feb 2025 at 01:26, Shubham Khanna
> wrote:
> >
> > On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
> > wrote:
> > >
> > > Hi Shubham,
> > >
> >
On Tue, Feb 4, 2025 at 5:39 PM Shlok Kyal wrote:
>
> On Wed, 29 Jan 2025 at 15:14, Shubham Khanna
> wrote:
> >
> > On Wed, Jan 29, 2025 at 10:42 AM Hayato Kuroda (Fujitsu)
> > wrote:
> > >
> > > Dear Shubham,
> > >
> > >
On Tue, Feb 4, 2025 at 3:49 PM Ashutosh Bapat
wrote:
>
> Hi Shubham,
>
> On Tue, Feb 4, 2025 at 2:10 PM Shubham Khanna
> wrote:
> > > >
> > >
> > > It could be a bit tricky to find that for users but they can devise a
> > > query to get
t; query to get the names and numbers of databases matching the given
> pattern. OTOH, I am not sure there is a clear need at this stage for
> pattern matching for this tool. So, we can go with a simple switch as
> you are proposing at this stage.
>
After reconsidering the idea of support
= off');
> +$node_a->start;
> +
> +# Set up node B as standby linking to node A
> +$node_a->backup('backup_3');
> +my $node_b = PostgreSQL::Test::Cluster->new('node_b');
> +$node_b->init_from_backup($node_a, 'backup_3', has_streaming => 1);
> +$node_
.blogspot.com/2024/09/online-upgrading-logical-and-physical.html
Thanks and regards,
Shubham Khanna.
v1-0001-Support-for-dropping-all-publications-in-pg_creat.patch
Description: Binary data
x27;--subscriber-port', $node_s->port,
> + '--publication', 'pub1',
> + '--publication', 'pub2',
> + '--subscription', 'sub1',
> + '--subscription', 'sub2',
> + '--database&#
. The attached patch has the changes for the
same.
[1] -
https://www.postgresql.org/message-id/CAA4eK1%2BJpALAokLqxVsQKgo9iFrO-zChfvNXXJMkC8jUgYykBw%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
v1-0001-Enhance-pg_createsubscriber-to-fetch-and-append-a.patch
Description: Binary data
ase find the updated patch.
Thanks and regards,
Shubham Khanna.
v12-0001-Validate-max_slot_wal_keep_size-in-pg_createsubs.patch
Description: Binary data
Hi,
Following the recent comments on Patch v13-0001, the patch was not
applied to the HEAD. I have addressed the feedback and prepared a
rebased version to incorporate the necessary adjustments.
Please find the updated patch.
Thanks and regards,
Shubham Khanna.
v14-0001-Add-support-for-two
pLUHCeDN%2Be5p0CsdQxcKEOiWRnRjxfg%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
On Mon, Jan 20, 2025 at 1:34 AM Shlok Kyal wrote:
>
> On Fri, 17 Jan 2025 at 09:52, Shubham Khanna
> wrote:
> >
> > On Fri, Jan 17, 2025 at 5:43 AM Peter Smith wrote:
> > >
> > > Hi Shubham,
> > >
> > > Some review comments
> 3b.
> I checked other PG source code, but I couldn't find any examples where
> the switch is given in single quotes quite like this.
>
> Maybe choose from one of the below instead:
>
> SUGGESTION #1
> Use the \"--enable-two-phase\" switch to enable two_phase.
&
On Thu, Jan 16, 2025 at 3:15 PM Shlok Kyal wrote:
>
> On Wed, 15 Jan 2025 at 12:03, Shubham Khanna
> wrote:
> >
> > On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal wrote:
> > >
> > > On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> > > wrote:
> >
On Thu, Jan 16, 2025 at 4:53 AM Peter Smith wrote:
>
> On Wed, Jan 15, 2025 at 9:24 PM Ajin Cherian wrote:
> >
> >
> >
> > On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna
> > wrote:
> > > Previously, the warning was necessary because the 'tw
On Wed, Jan 15, 2025 at 3:54 PM Ajin Cherian wrote:
>
>
>
> On Wed, Jan 15, 2025 at 5:33 PM Shubham Khanna
> wrote:
> > Previously, the warning was necessary because the 'two-phase' option
> > was not available, and users needed to be informed about the defa
On Thu, Jan 16, 2025 at 12:12 PM Shlok Kyal wrote:
>
> On Thu, 16 Jan 2025 at 10:48, Shubham Khanna
> wrote:
> >
> > On Wed, Jan 15, 2025 at 3:28 AM Peter Smith wrote:
> > >
> > > Hi Shubham.
> > >
> > > Patch v9-0001 LGTM.
> > >
rltidy' and made the necessary
adjustments.
The attached patch includes these formatting fixes along with the
latest changes.
Thanks and regards,
Shubham Khanna.
v10-0001-Validate-max_slot_wal_keep_size-in-pg_createsubs.patch
Description: Binary data
On Tue, Jan 14, 2025 at 4:53 PM Shlok Kyal wrote:
>
> On Fri, 27 Dec 2024 at 12:06, Shubham Khanna
> wrote:
> >
> > On Fri, Dec 27, 2024 at 11:30 AM vignesh C wrote:
> > > > > > >
> > > The documentation requires a minor update: instead of sp
er
> ...
> --
>
> See the attached diff for the TAP changes I used to expose this
> logging. Apply this atop v8.
>
> ==
> [1]
> https://www.postgresql.org/message-id/CAHut%2BPszk61QM%2BcEvq_1-A2y%2BJrAD0USB%2BNvtcidajYOfHDkyw%40mail.gmail.com
>
I have used your diff to update the TAP tests accordingly. The v9
version Patch attached at [1] has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ot;. The actual value
> is of no real importance to the patch, so going to extra trouble to
> extract an int64 that we don't care about seems unnecessary
>
I have fixed the given comment and used the string to accept the
value. The v9 version Patch attached at [1] has the changes for the
same.
[1] -
https://www.postgresql.org/message-id/CAHv8RjL4MFEDosz9%2BTFmufYCDoNCnyN6PFeSxG6MwsH1ufhZLA%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
-1 in the log file
> 'tmp_check/log/regress_log_040_pg_createsubscriber', which may not
> even be from the same test. Am I looking in the wrong place (???) e.g.
> Where is the logging evidence of that publisher's bad GUC (10MB) value
> in the logs before the warning is emitted?
>
I have fixed the given comments using your suggestions. The attached
patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v9-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
isn't it enough to
> use INT64_FORMAT macro?
>
> ```
> +# Configure 'max_slot_wal_keep_size = 1' on the publisher and
> +# reload configuration
> +$node_p->append_conf('postgresql.conf', 'max_slot_wal_keep_size = 1');
> +$node_p->rel
> pg_size_bytes()
> to get max_slot_wal_keep_size.
>
I have fixed the given comment. The v7 version Patch attached at [1]
has the changes for the same.
[1] -
https://www.postgresql.org/message-id/CAHv8Rj%2BSxmBgz-vw5f7meRzEh6pgp9YhAjL5BPzTpevu31rY7Q%40mail.gmail.com
Thanks and regards,
Shubham Khanna.
ggestion and updated the 'pg_createsubscriber'
documentation accordingly.
The attached Patch contains the suggested change.
Thanks and regards,
Shubham Khanna.
v7-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
_warning_details)
> The max_slot_wal_keep_size parameter is currently set to a non-default
> value, which may lead to replication failures. This parameter must be
> set to -1 to ensure that required WAL files are not prematurely
> removed.
>
> SUGGESTION (pg_log_warning_hint)
> Set the configuration parameter \"%s\" to -1 to ensure that required
> WAL files are not prematurely removed.
>
I have fixed the given comments using your suggestions. Tha attached
patch contains the suggested changes.
Thanks and Regards,
Shubham Khanna.
v6-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
om
> [2]
> https://www.postgresql.org/message-id/CALDaNm09cRzke52UN5zx33PT390whU92oXY4gfOSZEo17CLPjw%40mail.gmail.com
If 'max_slot_wal_keep_size' is not set to -1, there is no surety that
'pg_createsubscriber' will function as expected and the removal of WAL
files will start to occur. Therefore, a warning is raised instead of
an error to alert users about the potential configuration issue.
If 'max_slot_wal_keep_size' is -1 (the default), replication slots may
retain an unlimited amount of WAL files.
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v5-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
in
> that case it might be a poor user experience if we are effectively
> saying: "Too bad, it's all broken now; we warned you (only if you
> tried a dry run), but you did not listen".
>
> In other words, why not make this an error condition up-front to
> entirely remove any chance of this failure?
>
Changed the warning condition to the error condition and also updated
the test case accordingly.
> ==
The attached Patch contains the suggested changes.
Thanks and regards,
Shubham Khanna.
v4-0001-Validate-max_slot_wal_keep_size-in-pg_createsubsc.patch
Description: Binary data
On Mon, Dec 30, 2024 at 4:03 PM vignesh C wrote:
>
> On Mon, 30 Dec 2024 at 12:04, Shubham Khanna
> wrote:
> >
> > On Mon, Dec 30, 2024 at 10:10 AM vignesh C wrote:
> > >
> > > On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> > > wrote:
> >
On Mon, Dec 30, 2024 at 10:10 AM vignesh C wrote:
>
> On Mon, 30 Dec 2024 at 09:34, Shubham Khanna
> wrote:
> >
> > Hi,
> >
> > Currently, there is a risk that pg_createsubscriber may fail to
> > complete successfully when the max_slot_wal_keep_size valu
1 - 100 of 198 matches
Mail list logo
