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:
> > >
> > >
Thanks for providing the comments.
On Thu, 14 Nov 2024 at 12:22, vignesh C wrote:
>
> On Wed, 13 Nov 2024 at 11:15, Shlok Kyal wrote:
> >
> > Thanks for providing the comments.
> >
> > On Tue, 12 Nov 2024 at 12:52, Zhijie Hou (Fujitsu)
> > wrote:
> > &
Thanks for providing the comments.
On Tue, 12 Nov 2024 at 12:52, Zhijie Hou (Fujitsu)
wrote:
>
> On Friday, November 8, 2024 7:06 PM Shlok Kyal
> wrote:
> >
> > Hi Amit,
> >
> > On Thu, 7 Nov 2024 at 11:37, Amit Kapila wrote:
> > >
> > > On
Hi Amit,
On Thu, 7 Nov 2024 at 11:37, Amit Kapila wrote:
>
> On Tue, Nov 5, 2024 at 12:53 PM Shlok Kyal wrote:
> >
> > To avoid the issue, we can disallow UPDATE/DELETE on table with
> > unpublished generated column as REPLICA IDENTITY. I have attached a
> > patch
index t3_idx;
ALTER TABLE
postgres=# insert into t3 values(1);
INSERT 0 1
postgres=# update t3 set c1 = 100 where c1 = 1;
UPDATE 1
postgres=# create publication pub3 for table t3;
CREATE PUBLICATION
postgres=# update t3 set c1 = 100 where c1 = 1;
ERROR: cannot update table "t3"
DETAIL: Column list used by the publication does not cover the
replica identity.
So, I think this behavior would be acceptable. Thoughts?
[1]:
https://www.postgresql.org/message-id/CANhcyEVw4V2Awe2AB6i0E5AJLNdASShGfdBLbUd1XtWDboymCA%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
I have attached a
patch for the same.
[1]:
https://www.postgresql.org/message-id/CAD21AoA_RBkMa-6nUpBSoEP9s%3D46r3oq15vQkunVRCsYKXKMnA%40mail.gmail.com
Thanks and regards,
Shlok Kyal
v1-0001-Disallow-UPDATE-DELETE-on-table-with-generated-co.patch
Description: Binary data
rsue is to distribute invalidations in logical decoding
> > infrastructure [1] which has its downsides.
> >
> > Thoughts?
>
> Thank you for summarizing the problem and solutions!
>
> I think it's worth trying the idea of distributing invalidation
> messages, and we will see if there could be overheads or any further
> obstacles. IIUC this approach would resolve another issue we discussed
> before too[1].
>
> Regards,
>
> [1]
> https://www.postgresql.org/message-id/CAD21AoAenVqiMjpN-PvGHL1N9DWnHSq673bfgr6phmBUzx=k...@mail.gmail.com
>
Hi Sawada-san,
I have tested the scenario shared by you on the thread [1]. And I
confirm that the latest patch [2] fixes this issue.
[1]
https://www.postgresql.org/message-id/CAD21AoAenVqiMjpN-PvGHL1N9DWnHSq673bfgr6phmBUzx=k...@mail.gmail.com
[2]
https://www.postgresql.org/message-id/CANhcyEWfqdUvn2d2KOdvkhebBi5VO6O8J%2BC6%2BOwsPNwCTM%3DakQ%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
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.
I have applied the changes and updated the patch.
Thanks & Re
On Fri, 4 Oct 2024 at 12:52, Shlok Kyal wrote:
>
> Hi Kuroda-san,
>
> Thanks for reviewing the patch.
> >
> > 1.
> > I feel the name of SnapBuildDistributeNewCatalogSnapshot() should be
> > updated because it
> > distributes two objects: catalog
ing on the
'publish_via_partition_root' option. Will test and address this in the
next version of the patch. For now, I have added a TODO.
Thanks and Regards,
Shlok Kyal
v12-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch
Description: Binary data
v12-0002-Selective-Invalidation-of-Cache.patch
Description: Binary data
TO ..' is executed the
relcache is invalidated for that specific publication.
[1] :
https://www.postgresql.org/message-id/CANhcyEWEXL3rxvKH9-Xtx-DgGX0D62EktHpW%2BnG%2BMSSaMVUVig%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
RENAME TO ...' and
'ALTER PUBLICATION ... OWNER TO ...' and debugged it. The newly
added callback is called and it invalidates the cache of tables
present in that particular publication.
I have also added a test related to 'ALTER PUBLICATION ... RENAME TO
...' to 0001 patch.
Thanks and Regards,
Shlok Kyal
v11-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch
Description: Binary data
v11-0002-Selective-Invalidation-of-Cache.patch
Description: Binary data
On Thu, 26 Sept 2024 at 11:39, Shlok Kyal wrote:
>
> > In the v7 patch, I am looping through the reorder buffer of the
> > current committed transaction and storing all invalidation messages in
> > a list. Then I am distributing those invalidations.
> > But I foun
>
> IIUC, the executed workload did not contain ALTER SCHEMA command, so
> third improvement did not contribute this improvement.
I have removed the changes corresponding to the third improvement.
I have addressed the comment for 0002 patch and attached the patches.
Also, I have moved the tests in the 0002 to 0001 patch.
Thanks and Regards,
Shlok Kyal
v10-0002-Selective-Invalidation-of-Cache.patch
Description: Binary data
v10-0001-Distribute-invalidatons-if-change-in-catalog-tab.patch
Description: Binary data
11801.086 ms |20.60030913
512kb|12361.4172 ms |65.27390105
1MB |12343.3732 ms |80.84427202
2MB |12357.675 ms |79.40017604
4MB | 12395.8364 ms |76.78273689
8MB |11712.8862 ms |50.74323039
===
dation
[1]:https://www.postgresql.org/message-id/CANhcyEW4pq6%2BPO_eFn2q%3D23sgV1budN3y4SxpYBaKMJNADSDuA%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
From 4222dca86e4892fbae6698ed7a6135f61d499d8f Mon Sep 17 00:00:00 2001
From: Shlok Kyal
Date: Fri, 23 Aug 2024 14:02:20 +0530
Subject: [PATCH v9 1/2
On Mon, 9 Sept 2024 at 10:41, Shlok Kyal wrote:
>
> On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote:
> >
> > On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote:
> > >
> > > Next I am planning to test solely on the logical decoding side and
> > > will
On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote:
>
> On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote:
> >
> > Next I am planning to test solely on the logical decoding side and
> > will share the results.
> >
>
> Thanks, the next set of proposed tests makes s
On Mon, 2 Sept 2024 at 10:12, Amit Kapila wrote:
>
> On Fri, Aug 30, 2024 at 3:06 PM Shlok Kyal wrote:
> >
> > Next I am planning to test solely on the logical decoding side and
> > will share the results.
> >
>
> Thanks, the next set of proposed tests makes s
. with 'tcount = 10'. But I didn't find any performance impact.
2. with 'tcount = 0' and running the loop 1000 times. But I didn't
find any performance impact.
I have also attached the test script and the machine configurations on
which performance testing was done.
om the same problem?
I tried testing this scenario and I was able to reproduce the crash in
HEAD with this scenario. I have created a patch for the testcase.
I also tested the same scenario with the latest patch shared by
Sawada-san in [1]. And confirm that this resolves the issue.
[1]:
https://www.postgresql.org/message-id/CAD21AoDHC4Sob%3DNEYTxgu5wd4rzCpSLY_hWapMUqf4WKrAxmyw%40mail.gmail.com
Thanks and Regards,
Shlok Kyal
fix_memory_counter_update_in_reorderbuffer_v2.patch
Description: Binary data
test.patch
Description: Binary data
1 - 100 of 167 matches
Mail list logo
