Refreshing subscription relation state inside a transaction block

Started by Masahiko Sawadaover 8 years ago12 messages
#1Masahiko Sawada
sawada.mshk@gmail.com
1 attachment(s)

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

* ALTER SUBSCRIPTION SET PUBLICATION WITH (refresh = true)
* ALTER SUBSCRIPTION REFRESH PUBLICATION

Also, even if we rollback ALTER SUBSCRIPTION REFRESH PUBLICATION, the
error message "NOTICE: removed subscription for table public.t1"
appears. It's not a bug but might confuse the user.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

alter_subscription_and_transaction.patchapplication/octet-stream; name=alter_subscription_and_transaction.patchDownload
diff --git a/doc/src/sgml/ref/alter_subscription.sgml b/doc/src/sgml/ref/alter_subscription.sgml
index bead996..5f4d40a 100644
--- a/doc/src/sgml/ref/alter_subscription.sgml
+++ b/doc/src/sgml/ref/alter_subscription.sgml
@@ -48,6 +48,13 @@ ALTER SUBSCRIPTION <replaceable class="PARAMETER">name</replaceable> RENAME TO <
    (Currently, all subscription owners must be superusers, so the owner checks
    will be bypassed in practice.  But this might change in the future.)
   </para>
+
+  <para>
+   <command>ALTER SUBSCRIPTION ... SET PUBLCIATION</command> with
+   <literal>refresh</literal> option and
+   <command>ALTER SUBSCRIPTION ... REFRESH PUBLICATION</command> cannot be
+   executed inside a transaction block.
+  </para>
  </refsect1>
 
  <refsect1>
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5aae7b6..6f3f4a6 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -612,7 +612,7 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
  * Alter the existing subscription.
  */
 ObjectAddress
-AlterSubscription(AlterSubscriptionStmt *stmt)
+AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel)
 {
 	Relation	rel;
 	ObjectAddress myself;
@@ -743,6 +743,15 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
 				/* Refresh if user asked us to. */
 				if (refresh)
 				{
+					/*
+					 * Since stopping table sync worker is not transactional, the table
+					 * sync worker is stopped even if the transaction rolls back. So we
+					 * cannot run ALTER SUBSCRIPTION SET PUBLICATION with refresh option
+					 * and ALTER SUBSCRIPTION REFRESH PUBLICATION (see below) inside a
+					 * transaction block if refresh option is specified.
+					 */
+					PreventTransactionChain(isTopLevel, "ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = true)");
+
 					if (!sub->enabled)
 						ereport(ERROR,
 								(errcode(ERRCODE_SYNTAX_ERROR),
@@ -762,6 +771,12 @@ AlterSubscription(AlterSubscriptionStmt *stmt)
 			{
 				bool		copy_data;
 
+				/*
+				 * As same resason as above description at ALTER_SUBSCRIPTION_PUBLICATION,
+				 * we cannot run this command inside a transaction block
+				 */
+				PreventTransactionChain(isTopLevel, "ALTER SUBSCRIPTION ... REFRESH PUBLICATION");
+
 				if (!sub->enabled)
 					ereport(ERROR,
 							(errcode(ERRCODE_SYNTAX_ERROR),
diff --git a/src/backend/tcop/utility.c b/src/backend/tcop/utility.c
index a22fd53..db4e192 100644
--- a/src/backend/tcop/utility.c
+++ b/src/backend/tcop/utility.c
@@ -1626,7 +1626,7 @@ ProcessUtilitySlow(ParseState *pstate,
 				break;
 
 			case T_AlterSubscriptionStmt:
-				address = AlterSubscription((AlterSubscriptionStmt *) parsetree);
+				address = AlterSubscription((AlterSubscriptionStmt *) parsetree, isTopLevel);
 				break;
 
 			case T_DropSubscriptionStmt:
diff --git a/src/include/commands/subscriptioncmds.h b/src/include/commands/subscriptioncmds.h
index 1e4428e..f5a9e28 100644
--- a/src/include/commands/subscriptioncmds.h
+++ b/src/include/commands/subscriptioncmds.h
@@ -20,7 +20,7 @@
 
 extern ObjectAddress CreateSubscription(CreateSubscriptionStmt *stmt,
 				   bool isTopLevel);
-extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt);
+extern ObjectAddress AlterSubscription(AlterSubscriptionStmt *stmt, bool isTopLevel);
 extern void DropSubscription(DropSubscriptionStmt *stmt, bool isTopLevel);
 
 extern ObjectAddress AlterSubscriptionOwner(const char *name, Oid newOwnerId);
diff --git a/src/test/regress/expected/subscription.out b/src/test/regress/expected/subscription.out
index 4fcbf7e..6cc33a1 100644
--- a/src/test/regress/expected/subscription.out
+++ b/src/test/regress/expected/subscription.out
@@ -141,6 +141,16 @@ HINT:  The owner of a subscription must be a superuser.
 ALTER ROLE regress_subscription_user2 SUPERUSER;
 -- now it works
 ALTER SUBSCRIPTION testsub OWNER TO regress_subscription_user2;
+-- fail - cannot do ALTER SUBSCRIPTION ... SET PUBLICATION inside transaction block with refreshing
+BEGIN;
+ALTER SUBSCRIPTION testsub SET PUBLICATION testpub WITH (refresh = true);
+ERROR:  ALTER SUBSCRIPTION ... SET PUBLICATION ... WITH (refresh = true) cannot run inside a transaction block
+COMMIT;
+-- fail - cannot do ALTER SUBSCRIPTION ... REFRESH PUBLICATION inside transaction block
+BEGIN;
+ALTER SUBSCRIPTION testsub REFRESH PUBLICATION;
+ERROR:  ALTER SUBSCRIPTION ... REFRESH PUBLICATION cannot run inside a transaction block
+COMMIT;
 -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name
 BEGIN;
 DROP SUBSCRIPTION testsub;
diff --git a/src/test/regress/sql/subscription.sql b/src/test/regress/sql/subscription.sql
index 36fa1bb..aa14cae 100644
--- a/src/test/regress/sql/subscription.sql
+++ b/src/test/regress/sql/subscription.sql
@@ -102,6 +102,16 @@ ALTER ROLE regress_subscription_user2 SUPERUSER;
 -- now it works
 ALTER SUBSCRIPTION testsub OWNER TO regress_subscription_user2;
 
+-- fail - cannot do ALTER SUBSCRIPTION ... SET PUBLICATION inside transaction block with refreshing
+BEGIN;
+ALTER SUBSCRIPTION testsub SET PUBLICATION testpub WITH (refresh = true);
+COMMIT;
+
+-- fail - cannot do ALTER SUBSCRIPTION ... REFRESH PUBLICATION inside transaction block
+BEGIN;
+ALTER SUBSCRIPTION testsub REFRESH PUBLICATION;
+COMMIT;
+
 -- fail - cannot do DROP SUBSCRIPTION inside transaction block with slot name
 BEGIN;
 DROP SUBSCRIPTION testsub;
#2Petr Jelinek
petr.jelinek@2ndquadrant.com
In reply to: Masahiko Sawada (#1)
Re: Refreshing subscription relation state inside a transaction block

On 13/06/17 09:06, Masahiko Sawada wrote:

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

Can you be more specific than "It doesn't work fine inside transaction
block", what do you expect to happen and what actually happens?

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Petr Jelinek (#2)
Re: Refreshing subscription relation state inside a transaction block

On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 13/06/17 09:06, Masahiko Sawada wrote:

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

Can you be more specific than "It doesn't work fine inside transaction
block", what do you expect to happen and what actually happens?

If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
sync then it forcibly stops concurrently running table sync worker for
a table that had been removed from pg_subscription_rel. Then if we
rollback the transaction then these table sync worker start again from
the first. it's an rarely situation and not a damaging behavior but
what I expected is the table sync worker is stopped when the
refreshing transaction is committed.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Masahiko Sawada (#3)
Re: Refreshing subscription relation state inside a transaction block

On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 13/06/17 09:06, Masahiko Sawada wrote:

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

Can you be more specific than "It doesn't work fine inside transaction
block", what do you expect to happen and what actually happens?

If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
sync then it forcibly stops concurrently running table sync worker for
a table that had been removed from pg_subscription_rel.

Also, until commit the transaction the worker cannot launch new table
sync worker due to conflicting tuple lock on pg_subscription_rel.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Petr Jelinek
petr.jelinek@2ndquadrant.com
In reply to: Masahiko Sawada (#4)
Re: Refreshing subscription relation state inside a transaction block

On 13/06/17 18:33, Masahiko Sawada wrote:

On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Tue, Jun 13, 2017 at 4:53 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 13/06/17 09:06, Masahiko Sawada wrote:

Hi,

The commit ddd7b22b225ae41d16ceb218b387645cb9becfdc makes table sync
workers stop when subscription relation entry is removed. It doesn't
work fine inside transaction block. I think we should disallow to use
the following subscription DDLs inside a transaction block. Attached
patch.

Can you be more specific than "It doesn't work fine inside transaction
block", what do you expect to happen and what actually happens?

If we do ALTER SUBSCRIPTION SET PUBLICATION during executing table
sync then it forcibly stops concurrently running table sync worker for
a table that had been removed from pg_subscription_rel.

Hmm, forcibly stopping currently running table sync is not what was
intended, I'll have to look into it. We should not be forcibly stopping
anything except the main apply worker during drop subscription (and we
do that only because we can't drop the remote replication slot otherwise).

Also, until commit the transaction the worker cannot launch new table
sync worker due to conflicting tuple lock on pg_subscription_rel.

That part is correct, we don't know if the transaction will be rolled
back or not so we can't start workers as the state of the data in the
table in unknown.

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Peter Eisentraut
peter.eisentraut@2ndquadrant.com
In reply to: Petr Jelinek (#5)
Re: Refreshing subscription relation state inside a transaction block

On 6/15/17 02:41, Petr Jelinek wrote:

Hmm, forcibly stopping currently running table sync is not what was
intended, I'll have to look into it. We should not be forcibly stopping
anything except the main apply worker during drop subscription (and we
do that only because we can't drop the remote replication slot otherwise).

The change being complained about was specifically to address the
problem described in the commit message:

Stop table sync workers when subscription relation entry is removed

When a table sync worker is in waiting state and the subscription table
entry is removed because of a concurrent subscription refresh, the
worker could be left orphaned. To avoid that, explicitly stop the
worker when the pg_subscription_rel entry is removed.

Maybe that wasn't the best solution. Alternatively, the tablesync
worker has to check itself whether the subscription relation entry has
disappeared, or we need a post-commit check to remove orphaned workers.

--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#7Petr Jelinek
petr.jelinek@2ndquadrant.com
In reply to: Peter Eisentraut (#6)
Re: Refreshing subscription relation state inside a transaction block

On 15/06/17 15:52, Peter Eisentraut wrote:

On 6/15/17 02:41, Petr Jelinek wrote:

Hmm, forcibly stopping currently running table sync is not what was
intended, I'll have to look into it. We should not be forcibly stopping
anything except the main apply worker during drop subscription (and we
do that only because we can't drop the remote replication slot otherwise).

The change being complained about was specifically to address the
problem described in the commit message:

Stop table sync workers when subscription relation entry is removed

When a table sync worker is in waiting state and the subscription table
entry is removed because of a concurrent subscription refresh, the
worker could be left orphaned. To avoid that, explicitly stop the
worker when the pg_subscription_rel entry is removed.

Maybe that wasn't the best solution. Alternatively, the tablesync
worker has to check itself whether the subscription relation entry has
disappeared, or we need a post-commit check to remove orphaned workers.

Ah I missed that commit/discussion, otherwise I would have probably
complained. I don't like killing workers in the DDL command, as I said
the only reason we do it in DropSubscription is to free the slot for
dropping.

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#8Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Petr Jelinek (#7)
1 attachment(s)
Re: Refreshing subscription relation state inside a transaction block

On Thu, Jun 15, 2017 at 11:49 PM, Petr Jelinek
<petr.jelinek@2ndquadrant.com> wrote:

On 15/06/17 15:52, Peter Eisentraut wrote:

On 6/15/17 02:41, Petr Jelinek wrote:

Hmm, forcibly stopping currently running table sync is not what was
intended, I'll have to look into it. We should not be forcibly stopping
anything except the main apply worker during drop subscription (and we
do that only because we can't drop the remote replication slot otherwise).

The change being complained about was specifically to address the
problem described in the commit message:

Stop table sync workers when subscription relation entry is removed

When a table sync worker is in waiting state and the subscription table
entry is removed because of a concurrent subscription refresh, the
worker could be left orphaned. To avoid that, explicitly stop the
worker when the pg_subscription_rel entry is removed.

Maybe that wasn't the best solution. Alternatively, the tablesync
worker has to check itself whether the subscription relation entry has
disappeared, or we need a post-commit check to remove orphaned workers.

Ah I missed that commit/discussion, otherwise I would have probably
complained. I don't like killing workers in the DDL command, as I said
the only reason we do it in DropSubscription is to free the slot for
dropping.

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

Attachments:

check_subscription_rel_state_after_copy.patchapplication/octet-stream; name=check_subscription_rel_state_after_copy.patchDownload
diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index 5aae7b6..fbb0b96 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -597,8 +597,6 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 
 			RemoveSubscriptionRel(sub->oid, relid);
 
-			logicalrep_worker_stop(sub->oid, relid);
-
 			namespace = get_namespace_name(get_rel_namespace(relid));
 			ereport(NOTICE,
 					(errmsg("removed subscription for table %s.%s",
diff --git a/src/backend/replication/logical/tablesync.c b/src/backend/replication/logical/tablesync.c
index 3ff08bf..12e71ea 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -212,8 +212,8 @@ wait_for_relation_state_change(Oid relid, char expected_state)
  *
  * Used when transitioning from SYNCWAIT state to CATCHUP.
  *
- * Returns false if the apply worker has disappeared or the table state has been
- * reset.
+ * Returns false if the apply worker or the subscription relation state
+ * has disappeared or the table state has been reset.
  */
 static bool
 wait_for_worker_state_change(char expected_state)
@@ -223,9 +223,18 @@ wait_for_worker_state_change(char expected_state)
 	for (;;)
 	{
 		LogicalRepWorker *worker;
+		char	relstate;
+		XLogRecPtr	relstate_lsn;
 
 		CHECK_FOR_INTERRUPTS();
 
+		/* Check its state and died if it has disappeared */
+		relstate = GetSubscriptionRelState(MyLogicalRepWorker->subid,
+										   MyLogicalRepWorker->relid,
+										   &relstate_lsn, true);
+		if (relstate == SUBREL_STATE_UNKNOWN)
+			return false;
+
 		/* Bail if the apply has died. */
 		LWLockAcquire(LogicalRepWorkerLock, LW_SHARED);
 		worker = logicalrep_worker_find(MyLogicalRepWorker->subid,
@@ -914,8 +923,12 @@ LogicalRepSyncTableStart(XLogRecPtr *origin_startpos)
 				MyLogicalRepWorker->relstate_lsn = *origin_startpos;
 				SpinLockRelease(&MyLogicalRepWorker->relmutex);
 
-				/* Wait for main apply worker to tell us to catchup. */
-				wait_for_worker_state_change(SUBREL_STATE_CATCHUP);
+				/*
+				 * Wait for main apply worker to tell us to catchup. Exit
+				 * proc if the table sync worker is orphaned.
+				 */
+				if(!wait_for_worker_state_change(SUBREL_STATE_CATCHUP))
+					finish_sync_worker();
 
 				/*----------
 				 * There are now two possible states here:
#9Robert Haas
robertmhaas@gmail.com
In reply to: Masahiko Sawada (#8)
Re: Refreshing subscription relation state inside a transaction block

On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Do we still need to do something about this? Should it be an open item?

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#10Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Robert Haas (#9)
Re: Refreshing subscription relation state inside a transaction block

On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Do we still need to do something about this? Should it be an open item?

Thank you for looking at this.

Yeah, I think it should be added to the open item list. The patch is
updated by Petr and discussed on another thread[1]/messages/by-id/4c6e7ab3-091e-6336-df38-28937b153e64@2ndquadrant.com that also addresses
other issues of subscription codes. 0004 patch of that thread is an
updated patch of the patch attached on this thread.

[1]: /messages/by-id/4c6e7ab3-091e-6336-df38-28937b153e64@2ndquadrant.com

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#11Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Masahiko Sawada (#10)
Re: Refreshing subscription relation state inside a transaction block

On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Do we still need to do something about this? Should it be an open item?

Thank you for looking at this.

Yeah, I think it should be added to the open item list. The patch is
updated by Petr and discussed on another thread[1] that also addresses
other issues of subscription codes. 0004 patch of that thread is an
updated patch of the patch attached on this thread.

Does anyone have any opinions? Barring any objections, I'll add this
to the open item list.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#12Masahiko Sawada
sawada.mshk@gmail.com
In reply to: Masahiko Sawada (#11)
Re: Refreshing subscription relation state inside a transaction block

On Fri, Jul 28, 2017 at 1:13 PM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Thu, Jul 27, 2017 at 9:31 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

On Wed, Jul 26, 2017 at 10:29 PM, Robert Haas <robertmhaas@gmail.com> wrote:

On Mon, Jun 19, 2017 at 4:30 AM, Masahiko Sawada <sawada.mshk@gmail.com> wrote:

I think that either of the options you suggested now would be better.
We'll need that for stopping the tablesync which is in progress during
DropSubscription as well as those will currently still keep running. I
guess we could simply just register syscache callback, the only problem
with that is we'd need to AcceptInvalidationMessages regularly while we
do the COPY which is not exactly free so maybe killing at the end of
transaction would be better (both for refresh and drop)?

Attached patch makes table sync worker check its relation subscription
state at the end of COPY and exits if it has disappeared, instead of
killing table sync worker in DDL. There is still a problem that a
table sync worker for a large table can hold a slot for a long time
even after its state is deleted. But it would be for PG11 item.

Do we still need to do something about this? Should it be an open item?

Thank you for looking at this.

Yeah, I think it should be added to the open item list. The patch is
updated by Petr and discussed on another thread[1] that also addresses
other issues of subscription codes. 0004 patch of that thread is an
updated patch of the patch attached on this thread.

Does anyone have any opinions? Barring any objections, I'll add this
to the open item list.

Added it.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers