On Thu, 27 Jun 2024 at 08:38, Amit Kapila <amit.kapil...@gmail.com> wrote:
>
> On Wed, Jun 26, 2024 at 4:57 PM Tomas Vondra
> <tomas.von...@enterprisedb.com> wrote:
> >
> > On 6/25/24 07:04, Amit Kapila wrote:
> > > On Mon, Jun 24, 2024 at 8:06 PM Tomas Vondra
> > > <tomas.von...@enterprisedb.com> wrote:
> > >>
> > >> On 6/24/24 12:54, Amit Kapila wrote:
> > >>> ...
> > >>>>
> > >>>>>> I'm not sure there are any cases where using SRE instead of AE would
> > >>>>>> cause
> > >>>>>> problems for logical decoding, but it seems very hard to prove. I'd
> > >>>>>> be very
> > >>>>>> surprised if just using SRE would not lead to corrupted cache
> > >>>>>> contents in some
> > >>>>>> situations. The cases where a lower lock level is ok are ones where
> > >>>>>> we just
> > >>>>>> don't care that the cache is coherent in that moment.
> > >>>>
> > >>>>> Are you saying it might break cases that are not corrupted now? How
> > >>>>> could obtaining a stronger lock have such effect?
> > >>>>
> > >>>> No, I mean that I don't know if using SRE instead of AE would have
> > >>>> negative
> > >>>> consequences for logical decoding. I.e. whether, from a logical
> > >>>> decoding POV,
> > >>>> it'd suffice to increase the lock level to just SRE instead of AE.
> > >>>>
> > >>>> Since I don't see how it'd be correct otherwise, it's kind of a moot
> > >>>> question.
> > >>>>
> > >>>
> > >>> We lost track of this thread and the bug is still open. IIUC, the
> > >>> conclusion is to use SRE in OpenTableList() to fix the reported issue.
> > >>> Andres, Tomas, please let me know if my understanding is wrong,
> > >>> otherwise, let's proceed and fix this issue.
> > >>>
> > >>
> > >> It's in the commitfest [https://commitfest.postgresql.org/48/4766/] so I
> > >> don't think we 'lost track' of it, but it's true we haven't done much
> > >> progress recently.
> > >>
> > >
> > > Okay, thanks for pointing to the CF entry. Would you like to take care
> > > of this? Are you seeing anything more than the simple fix to use SRE
> > > in OpenTableList()?
> > >
> >
> > I did not find a simpler fix than adding the SRE, and I think pretty
> > much any other fix is guaranteed to be more complex. I don't remember
> > all the details without relearning all the details, but IIRC the main
> > challenge for me was to convince myself it's a sufficient and reliable
> > fix (and not working simply by chance).
> >
> > I won't have time to look into this anytime soon, so feel free to take
> > care of this and push the fix.
> >
>
> Okay, I'll take care of this.
This issue is present in all supported versions. I was able to
reproduce it using the steps recommended by Andres and Tomas's
scripts. I also conducted a small test through TAP tests to verify the
problem. Attached is the alternate_lock_HEAD.patch, which includes the
lock modification(Tomas's change) and the TAP test.
To reproduce the issue in the HEAD version, we cannot use the same
test as in the alternate_lock_HEAD patch because the behavior changes
slightly after the fix to wait for the lock until the open transaction
completes. The attached issue_reproduce_testcase_head.patch can be
used to reproduce the issue through TAP test in HEAD.
The changes made in the HEAD version do not directly apply to older
branches. For PG14, PG13, and PG12 branches, you can use the
alternate_lock_PG14.patch.
Regards,
Vignesh
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 6ea709988e..f4e5745909 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -1568,7 +1568,7 @@ OpenTableList(List *tables)
/* Allow query cancel in case this takes a long time */
CHECK_FOR_INTERRUPTS();
- rel = table_openrv(t->relation, ShareUpdateExclusiveLock);
+ rel = table_openrv(t->relation, ShareRowExclusiveLock);
myrelid = RelationGetRelid(rel);
/*
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index cb36ca7b16..3316e57ff5 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -487,6 +487,78 @@ $result =
is( $result, qq(2|f
3|t), 'check replicated update on subscriber');
+# Incremental data synchronization skipped when a new table is added, if
+# there is a concurrent active transaction involving the same table.
+
+# Create table in publisher and subscriber.
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_conc(a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_conc(a int)");
+
+# Bump the query timeout to avoid false negatives on slow test systems.
+my $psql_timeout_secs = 4 * $PostgreSQL::Test::Utils::timeout_default;
+
+# Initiate a background session that keeps a transaction active.
+my $background_psql1 = $node_publisher->background_psql(
+ 'postgres',
+ on_error_stop => 0,
+ timeout => $psql_timeout_secs);
+
+# Maintain an active transaction with the table.
+$background_psql1->set_query_timer_restart();
+$background_psql1->query_safe(
+ qq[
+ BEGIN;
+ INSERT INTO tab_conc VALUES (1);
+]);
+
+# Add the table to the publication using background_psql, as the alter
+# publication operation will wait for the lock and can only be completed after
+# the previous open transaction is committed.
+my $background_psql2 = $node_publisher->background_psql(
+ 'postgres',
+ on_error_stop => 0,
+ timeout => $psql_timeout_secs);
+
+$background_psql2->set_query_timer_restart();
+
+# This operation will wait because there is an open transaction holding a lock.
+$background_psql2->query_until(qr//,
+ "ALTER PUBLICATION pub1 ADD TABLE tab_conc;\n");
+
+# Verify that the table addition is waiting to acquire a ShareRowExclusiveLock.
+$node_publisher->poll_query_until('postgres',
+ "SELECT COUNT(1) = 1 FROM pg_locks WHERE relation = 'tab_conc'::regclass AND mode = 'ShareRowExclusiveLock' AND waitstart IS NOT NULL;"
+ )
+ or die
+ "Timed out while waiting for alter publication tries to wait on ShareRowExclusiveLock";
+
+# Complete the old transaction.
+$background_psql1->query_safe(qq[COMMIT]);
+$background_psql1->quit;
+
+$background_psql1->query_safe(qq[INSERT INTO tab_conc VALUES (2)]);
+$background_psql1->quit;
+
+# Refresh the publication.
+$node_subscriber->safe_psql('postgres',
+ 'ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION');
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab_conc");
+is( $result, qq(1
+2), 'Ensure that the data from the tab_conc table is synchronized to the subscriber after the subscription is refreshed');
+
+# Perform an insert.
+$node_publisher->safe_psql('postgres', "INSERT INTO tab_conc values(3)");
+$node_publisher->wait_for_catchup('sub1');
+
+# Verify that the insert is replicated to the subscriber.
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab_conc");
+is( $result, qq(1
+2
+3), 'Verify that the incremental data added after table synchronization is replicated to the subscriber');
+
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
diff --git a/src/test/subscription/t/100_bugs.pl b/src/test/subscription/t/100_bugs.pl
index cb36ca7b16..9fde78e1b9 100644
--- a/src/test/subscription/t/100_bugs.pl
+++ b/src/test/subscription/t/100_bugs.pl
@@ -487,6 +487,72 @@ $result =
is( $result, qq(2|f
3|t), 'check replicated update on subscriber');
+# Incremental data synchronization skipped when a new table is added, if
+# there is a concurrent active transaction involving the same table.
+
+# Create table in publisher and subscriber.
+$node_publisher->safe_psql('postgres', "CREATE TABLE tab_conc(a int)");
+$node_subscriber->safe_psql('postgres', "CREATE TABLE tab_conc(a int)");
+
+# Bump the query timeout to avoid false negatives on slow test systems.
+my $psql_timeout_secs = 4 * $PostgreSQL::Test::Utils::timeout_default;
+
+# Initiate a background session that keeps a transaction active.
+my $background_psql1 = $node_publisher->background_psql(
+ 'postgres',
+ on_error_stop => 0,
+ timeout => $psql_timeout_secs);
+
+# Maintain an active transaction with the table.
+$background_psql1->set_query_timer_restart();
+$background_psql1->query_safe(
+ qq[
+ BEGIN;
+ INSERT INTO tab_conc VALUES (1);
+]);
+
+# Add the table to the publication from background_psql
+my $background_psql2 = $node_publisher->background_psql(
+ 'postgres',
+ on_error_stop => 0,
+ timeout => $psql_timeout_secs);
+
+$background_psql2->set_query_timer_restart();
+
+# This will wait as the open transaction holding a lock.
+$background_psql2->query_until(qr//, "ALTER PUBLICATION pub1 ADD TABLE tab_conc;\n");
+
+$node_publisher->poll_query_until('postgres',
+"SELECT COUNT(1) = 1 FROM pg_publication_rel WHERE prrelid = 'tab_conc'::regclass;"
+ )
+ or die
+ "Timed out while waiting for the table tab_conc is added to pg_publication_rel";
+
+# Complete the old transaction.
+$background_psql1->query_safe(qq[COMMIT]);
+$background_psql1->quit;
+
+$background_psql1->query_safe(qq[INSERT INTO tab_conc VALUES (2)]);
+$background_psql1->quit;
+
+# Refresh the publication
+$node_subscriber->safe_psql('postgres',
+ 'ALTER SUBSCRIPTION sub1 REFRESH PUBLICATION');
+
+$node_subscriber->wait_for_subscription_sync($node_publisher, 'sub1');
+
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab_conc");
+is( $result, qq(1
+2), 'Ensure that the data from the tab_conc table is synchronized to the subscriber after the subscription is refreshed');
+
+$node_publisher->safe_psql('postgres', "INSERT INTO tab_conc values(3)");
+$node_publisher->wait_for_catchup('sub1');
+
+$result = $node_subscriber->safe_psql('postgres', "SELECT * FROM tab_conc");
+is( $result, qq(1
+2
+3), 'Verify that the incremental data added after table synchronization is replicated to the subscriber');
+
$node_publisher->stop('fast');
$node_subscriber->stop('fast');
diff --git a/src/backend/commands/publicationcmds.c b/src/backend/commands/publicationcmds.c
index 7ee8825522..55e8cbfdc9 100644
--- a/src/backend/commands/publicationcmds.c
+++ b/src/backend/commands/publicationcmds.c
@@ -571,7 +571,7 @@ OpenTableList(List *tables)
/* Allow query cancel in case this takes a long time */
CHECK_FOR_INTERRUPTS();
- rel = table_openrv(rv, ShareUpdateExclusiveLock);
+ rel = table_openrv(rv, ShareRowExclusiveLock);
myrelid = RelationGetRelid(rel);
/*
