quot;Set \"wal_level\" >= \"logical\" or create at least one
logical slot on the primary when \"wal_level\" = \"replica\".")));
4. In slotsync.c:
+ errmsg("replication slot synchronization requires
logical decoding to be enabled"),
+ errhint("Set \"wal_level\" >= \"logical\" or create at
least one logical slot with \"replica\" WAL level on the primary "));
Should we change the errhint message as below?
errhint("Set \"wal_level\" >= \"logical\" or create at least one
logical slot on the primary when \"wal_level\" = \"replica\".")));
-
I have also tested the patch with creating multiple permanent/
temporary slots in concurrent sessions and I did not find any issue. I
also tested this patch with a physical replication setup.
I have a doubt in this case:
1. Suppose we have a physical replication setup. wal_level on primary
is set to 'replica'
2. Now we try to create a logical slot on standby, an error is thrown
as wal_level is set to 'replica' as primary does not have any logical
slot
3. Now we create a temporary logical slot on primary,
effective_wal_level is set to logical.
4. Now we can create slots on standby as effective_wal_level is logical.
5. Now we exit the session of the primary server. The temporary slot
is dropped. This will invalidate the slots on standby as the
effective_wal_level will be set to 'replica'.
So we can say that indirectly a temporary slot on primary can control
the behaviour of permanent slots on standbys.
I checked this behaviour in HEAD. In HEAD also the behaviour is the
same. If we change the wal_level on primary from 'logical' to
'replica', all slots are invalidated on the standby.
With patch this behaviour can be indirectly controlled by a temporary
slot. Is it fine? Thoughts?
Thanks,
Shlok Kyal
tations as per the comments. The
changes are present in patch [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEXkeg3sjkS3DS9yU1ckz4ozUBNZ%2BRmrWaRNSSVCR8RquA%40mail.gmail.com
Thanks,
Shlok Kyal
subscriber,
replication happens as expected.
Also I found following documentation:
"Altering the publish_via_partition_root parameter can
lead to data loss or duplication at the subscriber because it changes
the identity and schema of the published tables. Note this happens only
when a partition root table is specified as the replication target."
Thanks,
Shlok Kyal
On Tue, 22 Jul 2025 at 14:29, shveta malik wrote:
>
> On Sat, Jul 19, 2025 at 4:17 PM Shlok Kyal wrote:
> >
> > On Mon, 30 Jun 2025 at 16:25, shveta malik wrote:
> > >
> > > Few more comments on 002:
> > >
> > > 5)
> > &g
= recoveryTargetXid)
> ```
>
> One difference between LSN and XID is the predictability. Regarding the WAL
> record, the startpoint of the next WAL record is surely next to the endpoint
> of
> previous WAL. In terms of transaction id, however, the commit ordering can be
> diff
On Mon, 21 Jul 2025 at 16:22, shveta malik wrote:
>
> On Sat, Jul 19, 2025 at 4:17 PM Shlok Kyal wrote:
> >
> > On Mon, 30 Jun 2025 at 16:25, shveta malik wrote:
> > >
> > > Few more comments on 002:
> > >
> > > 5)
> > &g
tion-row-filter.html
[3]:https://github.com/postgres/postgres/commit/7054186c4ebe24e63254651e2ae9b36efae90d4e
Thanks,
Shlok Kyal
v1-0001-Update-examples-in-logical-replication-docs.patch
Description: Binary data
dded the changes in the latest v16 patch [1].
[1]:https://www.postgresql.org/message-id/CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ%40mail.gmail.com
[2]:
https://docs.oracle.com/en/middleware/goldengate/core/23/reference/partition-partitionexclude.html
Thanks,
Shlok Kyal
sql.org/message-id/CANhcyEW2LK4diNeCG862DE40yQoV3VAgf59kXUq2TuR8fnw5vQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
On Mon, 30 Jun 2025 at 12:28, shveta malik wrote:
>
> On Fri, Jun 27, 2025 at 3:44 PM Shlok Kyal wrote:
> >
> > On Thu, 26 Jun 2025 at 15:27, shveta malik wrote:
> > >
> > > On Tue, Jun 24, 2025 at 9:48 AM Shlok Kyal
> > > wrote:
> > > &g
On Tue, 1 Jul 2025 at 08:20, Ajin Cherian wrote:
>
> On Mon, Jun 9, 2025 at 3:58 PM Shlok Kyal wrote:
> >
> > On Wed, 4 Jun 2025 at 16:12, Ajin Cherian wrote:
> > >
> > > On Tue, May 20, 2025 at 2:33 AM Shlok Kyal
> > > wrote:
> > > >
&
le
testing. Even in high end machines, it takes at least a few
microseconds. Also there are multiple cases where we did similar
timestamp comparison in 'stats.sql' as well. And, I didn't find any
other failure related to such case. So, I feel this is not possible.
2) pg_stat_reset_subscription_stats(oid) function did not reset the stats.
We have a shared hash 'pgStatLocal.shared_hash'. If the entry
reference (for the subscription) is not found while executing
'pg_stat_reset_subscription_stats(oid)'. It may not be able to reset
the stats. Maybe somehow this shared hash is getting dropped..
Also, it could be failing due to the same reason as Alexander has
reproduced in another case [2].
[1]:
https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=hamerkop&dt=2025-06-28%2011%3A02%3A30
[2]:
https://www.postgresql.org/message-id/fe0391a8-dfa9-41c3-bf1c-7ea058e40f30%40gmail.com
Thanks and Regards,
Shlok Kyal
On Thu, 26 Jun 2025 at 15:27, shveta malik wrote:
>
> On Tue, Jun 24, 2025 at 9:48 AM Shlok Kyal wrote:
> >
> > I have included the changes for
> > it in v14-0003 patch.
> >
> Thanks for the patches. I have reviewed patch001 alone, please find
> few comments:
Thanks for the comment, the attached v20250622 version patch has the
> changes for the same.
>
Hi Vignesh,
I tested with all patches applied. I have a comment:
Let consider following case:
On publisher create a publication pub1 on all sequence. publication
has sequence s1. The curr value of s1 is 2
and On subscriber we have subscription on pub1 and sequence s1 has
value 5. Now we run:
"ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION SEQUENCES"
Now on subscriber currval still show '5':
postgres=# select currval('s1');
currval
-
5
(1 row)
But when we do nextval on s1 on subscriber we get '3'. Which is
correct assuming sequence is synced:
postgres=# select nextval('s1');
nextval
-
3
(1 row)
postgres=# select currval('s1');
currval
-
3
(1 row)
Is this behaviour expected? I feel the initial " select
currval('s1');" should have displayed '2'. Thoughts?
Thanks and Regards,
Shlok Kyal
ables' with 'relations'
would be sufficient.
/*
* Build qsorted array of local table oids for faster lookup. This can
* potentially contain all tables in the database so speed of lookup
* is important.
*/
2. Similarly as above comment.
/*
* Walk over the remote tables and try to match them to locally known
* tables. If the table is not known locally create a new state for
* it.
*
* Also builds array of local oids of remote tables for the next step.
*/
3. Similarly as above comment.
/*
* Next remove state for tables we should not care about anymore using
* the data we collected above
*/
Similarly for above comment.
4. Since we are not adding sequences in the list 'sub_remove_rels',
should we only palloc for (the count of no. of tables)? Is it worth
the effort?
/*
* Rels that we want to remove from subscription and drop any slots
* and origins corresponding to them.
*/
sub_remove_rels = palloc(subrel_count * sizeof(SubRemoveRels));
Thanks and Regards,
Shlok Kyal
---++-
16415 | 16414 | 16388 ||
(1 row)
Here, we can set the publication to TABLE or TABLES FOR SCHEMA. Should
this be allowed?
If a publication is created on FOR ALL TABLES, such an operation is not allowed.
If we decide to allow it, it is currently not handled correctly as we
can still see "pub1" in Publications for sequence s1.
2. Similar to the comment given by Shveta in [1] point 3.
Same behaviour is present for ALTER PUBLICATION
postgres=# ALTER PUBLICATION pub2 ADD SEQUENCES;
ERROR: invalid publication object list
LINE 1: ALTER PUBLICATION pub2 ADD SEQUENCES;
^
DETAIL: One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
specified before a standalone table or schema name.
postgres=# ALTER PUBLICATION pub2 ADD TABLES;
ERROR: invalid publication object list
LINE 1: ALTER PUBLICATION pub2 ADD TABLES;
^
DETAIL: One of TABLE, TABLES IN SCHEMA or ALL SEQUENCES must be
specified before a standalone table or schema name.
[1]:
https://www.postgresql.org/message-id/CAJpy0uAY9sCRCkkVn4qbQeU8NMdLEA_wwBCyV0tvYft_sFST7g%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
o, I have modified v11-0001 to
RESET this as well.
I am also working on creating a patch to exclude columns in
publication as per suggestion in [2].
[1]:
https://www.postgresql.org/message-id/CALDaNm3dWZCYDih55qTNAYsjCvYXMFv%3D46UsDWmfCnXMt3kPCg%40mail.gmail.com
[2]:
https://www.postgresql.org/
On Wed, 4 Jun 2025 at 16:12, Ajin Cherian wrote:
>
> On Tue, May 20, 2025 at 2:33 AM Shlok Kyal wrote:
> >
> > This approach seems better to me. I have created a patch with the
> > above approach.
> >
> > Thanks and Regards,
> > Shlok Kyal
>
> S
in(), though for a different purpose. Along
> with it, we can still have a check during foreign table
> creation/attach that it doesn't become part of some publication, as
> the patch may have it now.
>
This approach seems better to me. I have created a patch with the
above approach.
On Mon, 28 Apr 2025 at 19:57, Álvaro Herrera wrote:
>
> On 2025-Apr-28, Shlok Kyal wrote:
>
> > 2.
> > + * We also take a ShareLock on pg_partitioned_table to restrict addition
> > + * of new partitioned table which may contain a foreign partition while
> > + * pu
On Mon, 7 Apr 2025 at 09:43, Sergey Tatarintsev
wrote:
>
> 07.04.2025 03:27, Álvaro Herrera пишет:
>
> On 2025-Apr-01, Shlok Kyal wrote:
>
> I have modified the comment in create_publication.sgml and also added
> comment in the restrictions section of logical-replication.sgml
essage-id/202504071239.kuj6m5a5wqxg%40alvherre.pgsql
[3]:
https://www.postgresql.org/message-id/c64352fa-9a30-4e0a-853a-a6b5b6d07f4e%40postgrespro.ru
[4]:
https://www.postgresql.org/message-id/CALDaNm2%2BeL22Sbvj74uS37xvt%3DhaQWcOwP15QnDuVeYsjHiffw%40mail.gmail.com
Thanks and Regards,
Shlok Kya
s.
> >
> > With only 0001, the walsender sends 'd4' insertion or later.
> >
> > WIth both 0001 and 0002, the wansender sends 'd3' insertion or later.
> >
> > ISTM the difference between without both 0001 and 0002 and with 0001
> > is signi
tname, s.subsynccommit,\n s.subpublications,\n s.subbinary,\n
s.substream,\n s.subtwophasestate,\n s.subdisableonerr,\n
s.subpasswordrequired,\n s.subrunasowner,\n s.suborigin,\n NULL AS
suboriginremotelsn,\n false AS subenabled,\n s.subfailover,\n NULL AS
subservername,\n NULL AS suboriginremotelsn,\n false AS subenabled,\n
false AS subfailover\nFROM pg_subscription s\nWHERE s.subdbid =
(SELECT oid FROM pg_database\n.."
is it expected?
Thanks and Regards,
Shlok Kyal
On Fri, 4 Apr 2025 at 10:36, Sergey Tatarintsev
wrote:
>
> 01.04.2025 21:48, Shlok Kyal пишет:
> > On Fri, 28 Mar 2025 at 16:35, Álvaro Herrera
> > wrote:
> >> On 2025-Mar-28, Shlok Kyal wrote:
> >>
> >>> On Mon, 24 Mar 2025 at 21:17, Álvaro Herr
XLogRecGetXid(r), &target_locator,
Since this comment is the same, should we remove it from the above
functions and add this where function 'SkipThisChange' is defined?
Thanks and Regards,
Shlok Kyal
build. Sorry for the
inconvenience.
[1]: https://www.postgresql.org/docs/devel/source-format.html
[2]:
https://www.postgresql.org/message-id/CANhcyEUF1HzDRj_fJAGCqBqNg7xGVoATR7XgR311xq8UvBg9tg%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
On Fri, 28 Mar 2025 at 16:35, Álvaro Herrera wrote:
>
> On 2025-Mar-28, Shlok Kyal wrote:
>
> > On Mon, 24 Mar 2025 at 21:17, Álvaro Herrera
> > wrote:
>
> > > Also, surely we should document this restriction in the SGML docs
> > >
; I think this
> castNode(RangeVar, lfirst(list_head(stmt->base.inhRelations)));
> is better written
> linitial_node(RangeVar, stmt->base.inhRelations);
Fixed.
I have attached the updated v10 patch with the above changes.
Thanks and Regards,
Shlok Kyal
v10-0001-Restrict-publishing-of-partitioned-table-with-fo.patch
Description: Binary data
node_p->safe_psql('postgres', "INSERT INTO tbl1 VALUES('first row')");
> > ```
> >
> > This is confusing because 'fourth row' is inserted to $db1 just after it.
> > How
> > about the 'row in database postgres' or something?
> >
>
> Fixed.
>
> The attached patches contain the suggested changes.
>
I have reviewed the v19-0001 patch I have a comment:
In pg_createsubscriber.sgml, this line seems little odd:
+ obtained from -P option. If the database name is not
+ specified in either the -d option, or the
+ -P option, and -a
+ an error will be reported.
Should we change the sentence to something like:
+ obtained from -P option. If the database name is not
+ specified in either the -d option, or the
+ -P option, and -a option is not
+ specified, an error will be reported.
Thanks,
Shlok Kyal
On Thu, 20 Feb 2025 at 21:18, vignesh C wrote:
>
> On Tue, 18 Feb 2025 at 15:59, Shlok Kyal wrote:
> >
> > On Mon, 17 Feb 2025 at 20:13, vignesh C wrote:
> > >
> > > On Fri, 14 Feb 2025 at 12:59, Shlok Kyal wrote:
> > > >
> > > > I
On Mon, 17 Mar 2025 at 22:57, Masahiko Sawada wrote:
>
> On Sun, Mar 9, 2025 at 11:16 PM Shlok Kyal wrote:
> >
> > On Fri, 28 Feb 2025 at 08:56, Amit Kapila wrote:
> > >
> > > On Fri, Feb 28, 2025 at 5:10 AM Masahiko Sawada
> > > wrote:
> >
On Mon, 17 Mar 2025 at 12:18, Amit Kapila wrote:
>
> On Mon, Mar 10, 2025 at 11:46 AM Shlok Kyal wrote:
> >
> >
> > I tried to reproduce the scenario described using the following steps:
> >
> > Here are the steps:
> >
> > 1. set max_rep
lot2' will be invalidated.
I have tried it with 'wal_removal' invalidation. So it is issue in
PG_13 to HEAD.
I also see a kill signal sent to function 'copy_replication_slot'.
Terminal output:
postgres=# select pg_copy_logical_replication_slot('slot2', 'slot_
On Wed, 11 Dec 2024 at 12:37, Masahiko Sawada wrote:
>
> On Tue, Oct 8, 2024 at 2:51 AM Shlok Kyal wrote:
> >
> > On Wed, 31 Jul 2024 at 03:27, Masahiko Sawada wrote:
> > >
> > > On Wed, Jul 24, 2024 at 9:53 PM Amit Kapila
> > > wrote:
> > &g
On Tue, 25 Feb 2025 at 01:03, Masahiko Sawada wrote:
>
> On Mon, Feb 24, 2025 at 3:06 AM Shlok Kyal wrote:
> >
> > On Sat, 22 Feb 2025 at 04:49, Masahiko Sawada wrote:
> > >
> > > On Fri, Feb 21, 2025 at 4:30 AM Shlok Kyal
> > > wrote:
> >
On Wed, 11 Dec 2024 at 09:13, Shlok Kyal wrote:
>
> On Tue, 8 Oct 2024 at 11:11, Shlok Kyal wrote:
> >
> > Hi Kuroda-san,
> >
> > > > I have also modified the tests in 0001 patch. These changes are only
> > > > related to syntax of writing tes
copy.
>
> SUGGESTION
>
> + /* Check if source slot became invalidated during the copy operation */
> + if (second_slot_contents.data.invalidated != RS_INVAL_NONE)
> + ereport(ERROR,
>
> ~
>
> 4b.
> Unnecessary parentheses in the ereport.
>
> ~
>
> 4c.
> There seems some weird mix of tense "cannot copy" versus "could not
> copy" already in this file. But, maybe at least you can be consistent
> within the patch and always say "cannot".
>
Fixed.
I have addressed the above comments in v5 patch [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEUHp6cRfaKf0ZqHCppCqpqzmsf5swpbnYGyRU%2BN%2Bihi%3DQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
On Sat, 22 Feb 2025 at 04:49, Masahiko Sawada wrote:
>
> On Fri, Feb 21, 2025 at 4:30 AM Shlok Kyal wrote:
> >
> > On Fri, 21 Feb 2025 at 01:14, Masahiko Sawada wrote:
> > >
> > > On Wed, Feb 19, 2025 at 3:46 AM Shlok Kyal
> > > wrote:
> > &
On Fri, 21 Feb 2025 at 01:14, Masahiko Sawada wrote:
>
> On Wed, Feb 19, 2025 at 3:46 AM Shlok Kyal wrote:
> >
> > On Tue, 18 Feb 2025 at 15:26, Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Monday, February 17, 2025 7:31 PM Shlok Kyal
> > >
On Thu, 20 Feb 2025 at 16:32, Zhijie Hou (Fujitsu)
wrote:
>
> On Wednesday, January 29, 2025 8:19 PM Shlok Kyal
> wrote:
> >
> > I have addressed the comments. Here is an updated patch.
>
> Thanks for updating the patch. The patches look mostly OK to me, I only have
&
walsender.c:3382
#27 0x5c9c56133895 in WalSndLoop (send_data=0x5c9c56135836
) at walsender.c:2788
#28 0x5c9c56130762 in StartLogicalReplication (cmd=0x5c9c58ab9a10)
at walsender.c:1496
#29 0x5c9c56131ec3 in exec_replication_command (
cmd_string=0x5c9c58a83b90 "START_REPLICATION SLOT \"test1\"
LOGICAL 0/0 (proto_version '4', streaming 'parallel', origin 'any',
publication_names '\"pub1\"')") at walsender.c:2119
#30 0x5c9c562abbe8 in PostgresMain (dbname=0x5c9c58abd598
"postgres", username=0x5c9c58abd580 "ubuntu") at postgres.c:4687
Thanks and Regards,
Shlok Kyal
On Tue, 18 Feb 2025 at 15:26, Zhijie Hou (Fujitsu)
wrote:
>
> On Monday, February 17, 2025 7:31 PM Shlok Kyal
> wrote:
> >
> > On Thu, 13 Feb 2025 at 15:54, vignesh C wrote:
> > >
> > > On Tue, 4 Feb 2025 at 15:27, Shlok Kyal wrote:
> > > >
On Mon, 17 Feb 2025 at 20:13, vignesh C wrote:
>
> On Fri, 14 Feb 2025 at 12:59, Shlok Kyal wrote:
> >
> > I have used the changes suggested by you. Also I have updated the
> > comments and the function name.
>
> There is another concurrency issue possible:
> +/*
On Thu, 13 Feb 2025 at 15:54, vignesh C wrote:
>
> On Tue, 4 Feb 2025 at 15:27, Shlok Kyal wrote:
> >
> > Hi,
> >
> > Currently, we can copy an invalidated slot using the function
> > 'pg_copy_logical_replication_slot'. As per the suggestion in the
recovery/t/035_standby_logical_decoding.pl
>
> 3.
> +# Attempting to copy an invalidated slot
> +($result, $stdout, $stderr) = $node_standby->psql(
>
> /Attempting/Attempt/
>
Fixed
> ======
> [1] https://www.postgresql.org/docs/current/functions-admin.html
I have updated the changes in v2 patch [1].
[1]:
https://www.postgresql.org/message-id/CANhcyEVJpb6%2Bhnk4MPVU3hZBYL%3DDS4v-PYBZOUoiivrN8Vd_Bw%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
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 Tue, Feb 11, 2025 at 9:56 PM Shlok Kyal
> > > wrote:
&g
On Thu, 13 Feb 2025 at 20:12, vignesh C wrote:
>
> On Thu, 13 Feb 2025 at 15:50, Shlok Kyal wrote:
> >
> >
> > I have fixed the issue. Attached the updated v6 patch.
>
> There is another concurrency issue:
> In case of create publication for all tables with
>
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:
> > >
> > > On Fri, Feb 7, 2025 at 7:46 AM Hayato Kuroda (Fujitsu)
On Thu, 13 Feb 2025 at 11:25, vignesh C wrote:
>
> On Tue, 11 Feb 2025 at 16:55, Shlok Kyal wrote:
> > I have handled the above cases and added tests for the same.
>
> There is a concurrency issue with the patch:
> +check_partrel_has_foreign_table(Form_pg_class relform
ackup/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
>
Hi Shubham,
I reviewed v6 patch, here are some of my comments:
1. Option 'P' is for "publisher-server".
case 'P':
+ if (opt.all_databases)
+ {
+ pg_log_error("--publication cannot be used with
--all-databases");
+ exit(1);
+ }
So, if we provide the "--all-databases" option and then the
"--publisher-server" option. Then the command pg_createsubscriber is
failing, which is wrong.
2. In case we provide "--all-database" option and then "--publication"
option, the pg_createsubscriber command is running. I think the above
condition should be added at "case 2:" instead of "case 'P':"
Thanks and Regards,
Shlok Kyal
> > [3]: https://www.postgresql.org/docs/devel/source-format.html
> >
>
> Fixed.
>
> The attached Patch contains the suggested changes.
>
Hi Shubham,
I have some comments for v4 patch:
1. I think we should update the comment for the function
'drop_publication'. As its usage is changed with this patch
Currently it states:
/*
* Remove publication if it couldn't finish all steps.
*/
2. In case when --cleanup_existing_publications is not specified the
info message has two double quotes.
pg_createsubscriber: dropping publication
""pg_createsubscriber_5_aa3c31f2"" in database "postgres"
The code:
+ appendPQExpBufferStr(targets,
+PQescapeIdentifier(conn, dbinfo->pubname,
+ strlen(dbinfo->pubname)));
It is appending the value along with the double quotes. I think we
should assign the value of 'PQescapeIdentifier(conn, dbinfo->pubname,
strlen(dbinfo->pubname)' to a string and then use it.
Thanks and Regards,
Shlok Kyal
On Mon, 10 Feb 2025 at 16:11, vignesh C wrote:
>
> On Tue, 4 Feb 2025 at 18:31, vignesh C wrote:
> >
> > On Thu, 30 Jan 2025 at 17:32, Shlok Kyal wrote:
> > >
> > > @@ -1428,6 +1427,12 @@ check_foreign_tables_in_schema(Oid schemaid)
> > >
s for the same.
> (If we later figure out a way to allow publish_via_partition_root and
> skip the tuples in foreign partitions, it's easy to remove the
> restriction.)
>
Thanks and Regards,
Shlok Kyal
v5-0001-Restrict-publishing-of-partitioned-table-with-for.patch
Description: Binary data
= cell->next;
+ }
Since we are not updating these counts, the names are reflected as expected.
Should also store subscription names similarly? Or maybe store
subscription names and assign slot names same as subscription names?
5. Since --all-databases option is added we should update the comment:
/*
* If --database option is not provided, try to obtain the dbname from
* the publisher conninfo. If dbname parameter is not available, error
* out.
*/
6. Extra blank lines should be removed
}
}
+
+
/* Number of object names must match number of databases */
Thanks and Regards,
Shlok Kyal
ations()
> is called. Cleanup operations should not affect the final result.
> drop_primary_replication_slot() and drop_failover_replication_slots() raise
> WARNING
> when they fail to drop objects because they are just cleanup functions.
> I feel we can follow this manner.
>
Hi Kuroda-san,
I agree with you. Raising WARNING makes sense to me.
Thanks and Regards,
Shlok Kyal
_OK)
+ {
+ pg_log_warning("could not obtain publication information: %s",
+ PQresultErrorMessage(res));
+
5. Should we throw an error in this case as well?
+ if (PQresultStatus(res_for_drop) != PGRES_COMMAND_OK)
+ {
+ pg_log_warning("could not drop publication \"%s\": %s",
+ pubname, PQresultErrorMessage(res));
+ }
6. We should start the standby server before creating the
publications, so that the publications are replicated to standby.
+# Create publications to test it's removal
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub FOR ALL TABLES;");
+$node_p->safe_psql($db1, "CREATE PUBLICATION test_pub2 FOR ALL TABLES;");
+
+# Verify the existing publications
+my $pub_count_before =
+ $node_p->safe_psql($db1, "SELECT COUNT(*) FROM pg_publication;");
Also maybe we should check the publication count on the standby node
instead of primary.
Thanks and Regards,
Shlok Kyal
On Tue, 4 Feb 2025 at 10:45, Amit Kapila wrote:
>
> On Mon, Feb 3, 2025 at 6:35 PM Shlok Kyal wrote:
> >
> > I reviewed the v66 patch. I have few comments:
> >
> > 1. I also feel the default value should be set to '0' as suggested by
> > Vignesh in
/CAA4eK1Kw%3DvZ2FZ4DdrmOhuxOAL%3D2abaBm8hu_PsVN2Hd6UFP-w%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
v1-0001-Restrict-copying-of-invalidated-replication-slots.patch
Description: Binary data
ical_replication_slot('test1', 'test2');
pg_copy_logical_replication_slot
------
(test2,0/16FDE18)
(1 row)
postgres=# select slot_name, active, restart_lsn, wal_status,
inactive_since , invalidation_reason from pg_replication_slots;
slot_name | active | restart_lsn | wal_status |
inactive_since | invalidation_reason
---++-++--+-
test1 | f | 0/16FDDE0 | lost | 2025-02-03
18:28:01.802463+05:30 | idle_timeout
test2 | f | 0/16FDDE0 | reserved | 2025-02-03
18:29:53.478023+05:30 |
(2 rows)
3. We have similar behaviour as above for physical slots.
[1]:
https://www.postgresql.org/message-id/CALDaNm14QrW5j6su%2BEAqjwnHbiwXJwO%2Byk73_%3D7yvc5TVY-43g%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
On Wed, 29 Jan 2025 at 19:21, Sergey Tatarintsev
wrote:
>
>
> 29.01.2025 12:16, Shlok Kyal пишет:
> > Hi,
> >
> > As part of a discussion in [1], I am starting this thread to address
> > the issue reported for foreign tables.
> >
> > Logical replicati
On Wed, 29 Jan 2025 at 15:58, vignesh C wrote:
>
> On Fri, 24 Jan 2025 at 09:52, Shlok Kyal wrote:
> >
> > I have added the test in the latest patch.
>
> Few comments:
> 1) Let's rearrange this query slightly so that the "PT.pubname IN
> (<pub-names&g
existing published tables.
[1] :
https://www.postgresql.org/message-id/CAA4eK1Lhh4SgiYQLNiWSNKGdVSzbd53%3Dsr2tQCKooEphDkUtgw%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
v1-0001-Restrict-publishing-of-partitioned-table-with-a-f.patch
Description: Binary data
used by the publication does not cover the
replica identity.
----
Test 5: Update publication on non virtual gen with no column list specified
CREATE TABLE t1 (a int, b int GENERATED ALWAYS AS (a * 2) VIRTUAL);
create publication pub1 for table t1;
alter table t1 replica identity full;
update t1 set a = 10;
No error is thrown, and an update is happening. It should have thrown
an ERROR as the unpublished generated column 'b' is part of the
replica identity.
Thanks and Regards,
Shlok Kyal
ind the updated patch.
>
> Thanks and regards,
> Shubham Khanna.
Hi Shubham,
I reviewed the patch and it looks good to me.
Thanks and Regards,
Shlok Kyal
On Thu, 23 Jan 2025 at 17:54, Zhijie Hou (Fujitsu)
wrote:
>
> On Thursday, January 23, 2025 4:43 PM Shlok Kyal
> wrote:
> >
> > On Thu, 23 Jan 2025 at 12:35, Shlok Kyal wrote:
> > >
> > > On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
> > &
but this time only for
> virtual generated columns, and amends the documentation. It doesn't
> rename the publication option "publish_generated_columns", but maybe
> that should be done.
Hi Peter,
I tried to apply the patch on HEAD but it is not applying.
Rebase is required because of recent commits.
Thanks and Regards,
Shlok Kyal
On Thu, 23 Jan 2025 at 12:35, Shlok Kyal wrote:
>
> On Wed, 22 Jan 2025 at 09:00, Zhijie Hou (Fujitsu)
> wrote:
> >
> > On Tuesday, January 21, 2025 1:31 AM vignesh C wrote:
> >
> > Hi,
> >
> > >
> > > On Mon, 20 Jan 2025 at 17:31, Amit
separate issue which can be fixed independtly.
>
> Hi Sergey, if you have the time, could you please verify whether this patch
> resolves the partition issue you reported? I've confirmed that it passes the
> partitioned tests in the scripts, but I would appreciate your confirmation for
separate issue which can be fixed independtly.
>
> Hi Sergey, if you have the time, could you please verify whether this patch
> resolves the partition issue you reported? I've confirmed that it passes the
> partitioned tests in the scripts, but I would appreciate your confirmation for
" switch to enable two_phase.
> >
> > SUGGESTION #2
> > Use the --enable-two-phase switch to enable two_phase.
> >
>
> Fixed the given comments. The attached patch contains the required changes.
>
Hi Shubham,
I have a comment for the v12 patch.
I think we can pass just the two_phase option instead of the whole
structure here. As we are just using 'opt->two_phase'.
+check_publisher(const struct LogicalRepInfo *dbinfo,
+ const struct CreateSubscriberOptions *opt)
Thanks and Regards,
Shlok Kyal
On Thu, 16 Jan 2025 at 12:35, Nisha Moond wrote:
>
> On Wed, Jan 15, 2025 at 11:37 AM Shlok Kyal wrote:
> >
> > On Thu, 2 Jan 2025 at 15:57, Nisha Moond wrote:
> > >
> > > On Thu, Jan 2, 2025 at 8:16 AM Peter Smith wrote:
> > > >
> > &g
On Thu, 16 Jan 2025 at 12:34, Shubham Khanna
wrote:
>
> 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:
> > >
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 Fri, Dec 27, 2024 at 11:30 AM vignesh C wrote:
> > > &
his, the source server must set
+ to -1 to ensure that required WAL files are not
+ prematurely removed.
For the above added line, we should add a space before each line.
Thanks and Regards,
Shlok Kyal
ULL, the
- * publish_generated_columns option must be set to true if the table
- * has any stored generated columns.
+ * publish_generated_columns option must be set to 's'(stored) if the
+ * table has any stored generated columns.
*/
- if (!pubgencols &&
+ if (gencols_type == PUBLISH_GENCOL_NONE &&
relation->rd_att->constr &&
To be consistent with the comment, I think we should check if
'gencols_type != PUBLISH_GENCOL_STORED' instead of 'gencols_type ==
PUBLISH_GENCOL_NONE'.
Thoughts?
Thanks and Regards,
Shlok Kyal
e the sleep below.
*/
We should not enter the if condition for the case when the slot was
already acquired by our process.
Thanks and Regards,
Shlok Kyal
e replicated at the time
of COMMIT PREPARED, without advance preparation.
Once setup is complete, you can manually drop and re-create the
subscription(s) with
the two_phase
option enabled.
I think we should update the documentation accordingly.
Thanks and Regards,
Shlok Kyal
nd of table synchronization worker process" seems
> > misleading. I also thought maybe it is better to mention that this is
> > PER table.
> >
> > SUGGESTION:
> > ... a special table synchronization worker process per table.
>
> Thanks, the updated v3 version patch has the changes for the same.
>
Hi Vignesh,
I reviewed the v3 patch. And it looks good to me.
Thanks and Regards,
Shlok Kyal
the time to clean up the spill files, I run the perl script and
then check the publisher log for the time of cleanup of the very first
transaction.
Thanks and Regards,
Shlok Kyal
cleanup_logs.patch
Description: Binary data
101_test.pl
Description: Binary data
case?
Also, which version are you facing this issue?
Have you set any GUC parameters/ Encoding/ Collation?
Thanks and Regards,
Shlok Kyal
#!/usr/bin/env bash
# setup publisher node
./pg_ctl stop -D ../publisher
rm -rf ../publisher publisher.log
mkdir ../publisher
./initdb -D ../publisher
cat <<
source code and check. I
> have created this patch on top of Postgres 15.6.
>
> [1]:
> https://www.postgresql.org/message-id/1430556325.185731745.1731484846410.javamail.zim...@meteo.fr
>
>
> Thanks and Regards,
> Shlok Kyal
>
> Thanks for the parch.
>
> I tried
ling unlink for every wal segment, we
are first reading the directory and filtering the files related to our
transaction and then unlinking those files.
You can apply the patch on your publisher source code and check. I
have created this patch on top of Postgres 15.6.
[1]:
https://www.postgresql.org/message-i
branches after applying patches in [2] and ran
the script 50 times. I was not able to reproduce the issue with patch.
I think as Hou-san suggested, the patches in [2] can fix this issue.
[1]:
https://www.postgresql.org/message-id/8b595156-d8b6-4b53-a788-7d945726cd2f%40pritambaral.com
[2]:
https://www.postgresql.org/message-id/flat/de52b282-1166-1180-45a2-8d8917ca74c6%40enterprisedb.com
Thanks and Regards,
Shlok Kyal
On Tue, 8 Oct 2024 at 11:11, Shlok Kyal wrote:
>
> Hi Kuroda-san,
>
> > > I have also modified the tests in 0001 patch. These changes are only
> > > related to syntax of writing tests.
> >
> > LGTM. I found small improvements, please find the attached.
>
ot; version feels more
> > > like it implies the Replica Identify is in the wrong, whereas I think
> > > it is most likely that the Replica Identity is correct, and the real
> > > problem is that the user just forgot to publish the generated column.
> > >
> >
> > Either way is possible and I find the message "Replica identity must
> > not contain unpublished generated columns." clearer as compared to the
> > other option.
> >
>
> OK, let's go with that.
>
Thanks Peter, for pointing this out.
I also feel that the error message suggested by Amit would be better.
I have attached a patch for the same.
Thanks and Regards,
Shlok Kyal
v1-0001-Improve-error-message-introduced-in-commit-87ce27.patch
Description: Binary data
On Tue, 3 Dec 2024 at 10:21, Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, November 29, 2024 9:08 PM Shlok Kyal
> wrote:
> >
> >
> > Thanks for reviewing the patch. I have fixed the issue and updated the
> > patch.
>
> Thanks for updating the patch. I
On Fri, 29 Nov 2024 at 15:49, vignesh C wrote:
>
> On Fri, 29 Nov 2024 at 13:38, Shlok Kyal wrote:
> >
> > On Thu, 28 Nov 2024 at 16:38, Amit Kapila wrote:
> > >
> > > On Thu, Nov 21, 2024 at 5:30 PM Shlok Kyal
> > > wrote:
> > > >
&
On Thu, 28 Nov 2024 at 16:38, Amit Kapila wrote:
>
> On Thu, Nov 21, 2024 at 5:30 PM Shlok Kyal wrote:
> >
>
> Review comments:
> ===
> 1.
> +
> + /*
> + * true if all generated columns which are part of replica identity are
> + * published or th
On Thu, 21 Nov 2024 at 15:26, vignesh C wrote:
>
> On Tue, 19 Nov 2024 at 19:12, Shlok Kyal wrote:
> >
> > On Tue, 19 Nov 2024 at 14:39, Zhijie Hou (Fujitsu)
> > wrote:
> > >
> > > On Tuesday, November 19, 2024 3:15 PM Shlok Kyal
> > > wro
ql-15/bin/postgres(PostmasterMain+0xe6c) [0x74d66c]
> > /usr/pgsql-15/bin/postgres(main+0x1c5) [0x494a05]
> > /usr/lib64/libc-2.17.so(__libc_start_main+0xf4) [0x22554]
> > /usr/pgsql-15/bin/postgres(_start+0x28) [0x494fb8]
>
> We did not find any other option than deleting the subscription to stop that
> loop and start a new one (thus loosing transactions).
>
> The publisher is PostgreSQL 15.6
> The subscriber is PostgreSQL 14.5
>
> Thanks
Hi,
Do you have a reproducible test case for the above scenario? Please
share the same.
I am also trying to reproduce the above issue by generating large no.
of spill files.
Thanks and Regards,
Shlok Kyal
On Tue, 19 Nov 2024 at 14:39, Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, November 19, 2024 3:15 PM Shlok Kyal
> wrote:
>
> >
> > I noticed that we can add 'publish_generated_columns = true' for the case of
> > generated column. So we won't need to
On Tue, 19 Nov 2024 at 10:22, vignesh C wrote:
>
> On Tue, 19 Nov 2024 at 00:36, Shlok Kyal wrote:
> >
> > On Mon, 18 Nov 2024 at 19:19, vignesh C wrote:
> > >
> > > On Mon, 18 Nov 2024 at 13:07, Shlok Kyal wrote:
> > > >
> > > > T
On Tue, 19 Nov 2024 at 09:50, Zhijie Hou (Fujitsu)
wrote:
>
> On Tuesday, November 19, 2024 3:06 AM Shlok Kyal
> wrote:
> >
> > I have fixed the comments and attached an updated patch.
>
> Thanks for the patch.
>
> I slightly refactored the cod
On Mon, 18 Nov 2024 at 19:19, vignesh C wrote:
>
> On Mon, 18 Nov 2024 at 13:07, Shlok Kyal wrote:
> >
> > Thanks for providing the comments.
> >
> > On Sat, 16 Nov 2024 at 17:29, vignesh C wrote:
> >
> > I have attached the updated version of the
On Mon, 18 Nov 2024 at 08:57, Zhijie Hou (Fujitsu)
wrote:
>
> On Saturday, November 16, 2024 2:41 AM Shlok Kyal
> wrote:
> >
> > >
> > Thanks for providing the comments. I have fixed all the comments and
> > attached
> > the updated patch.
>
>
Thanks for providing the comments.
On Sat, 16 Nov 2024 at 17:29, vignesh C wrote:
>
> On Sat, 16 Nov 2024 at 00:10, Shlok Kyal wrote:
> >
> > Thanks for providing the comments. I have fixed all the comments and
> > attached the updated patch.
&g
efore.
> > b) This current patch has overlapping logic so you need to be assured
> > that adding this new error doesn't break the existing one.
> > c) Only one of these errors wins. Adding both tests will define the
> > expected order if both error scenarios exist at the same time.
> >
>
> I have fixed the given comments. The attached Patch contains the
> required changes.
>
Thanks for providing the patch.
I have few comments:
1. Getting segmentation fault for following test case:
Publisher:
CREATE TABLE t1 (a INT, b INT);
create publication pub1 for table t1(b)
Subscriber:
CREATE TABLE t1 (a INT, b int GENERATED ALWAYS AS (a + 1) STORED NOT NULL)
create subscription test1 connection 'dbname=postgres host=localhost
port=5432' publication pub1
Subscriber logs:
2024-11-16 17:23:16.919 IST [3842385] LOG: logical replication apply
worker for subscription "test1" has started
2024-11-16 17:23:16.931 IST [3842389] LOG: logical replication table
synchronization worker for subscription "test1", table "t1" has
started
2024-11-16 17:29:47.855 IST [3842359] LOG: background worker "logical
replication tablesync worker" (PID 3842389) was terminated by signal
11: Segmentation fault
2024-11-16 17:29:47.856 IST [3842359] LOG: terminating any other
active server processes
2.
+ initStringInfo(&attsbuf);
'attsbuf' not free'd. I think we should pfree it.
Thanks and Regards,
Shlok Kyal
On Fri, 15 Nov 2024 at 20:31, vignesh C wrote:
>
> On Fri, 15 Nov 2024 at 16:45, Shlok Kyal wrote:
> >
> > Thanks for providing the comments
> >
> > On Fri, 15 Nov 2024 at 10:59, vignesh C wrote:
> > >
> > > On Thu, 14 Nov 2024 at 15:51, Shlok K
Thanks for providing the comments
On Fri, 15 Nov 2024 at 10:59, vignesh C wrote:
>
> On Thu, 14 Nov 2024 at 15:51, Shlok Kyal wrote:
> >
> > Thanks for providing the comments.
> >
> > On Thu, 14 Nov 2024 at 12:22, vignesh C wrote:
> > >
> > >
1 - 100 of 187 matches
Mail list logo
