Subject: Refreshing subscription relation state inside a transaction block

From: Masahiko Sawada <[email-masked]>
Date: Tue, 13 Jun 2017 16:06:27 +0900

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

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

---

From: Petr Jelinek <[email-masked]>
Date: Tue, 13 Jun 2017 09:53:23 +0200

> the following subscription DDLs inside a transaction block. Attached

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

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 14 Jun 2017 01:02:13 +0900

> 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,

---

From: Masahiko Sawada <[email-masked]>
Date: Wed, 14 Jun 2017 01:33:12 +0900

On Wed, Jun 14, 2017 at 1:02 AM, Masahiko Sawada <[email-masked]> wrote:
> 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,

---

From: Petr Jelinek <[email-masked]>
Date: Thu, 15 Jun 2017 08:41:16 +0200

>> 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).

> 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.

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 15 Jun 2017 09:52:27 -0400

> 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

---

From: Petr Jelinek <[email-masked]>
Date: Thu, 15 Jun 2017 16:49:35 +0200

> 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)?

---

From: Masahiko Sawada <[email-masked]>
Date: Mon, 19 Jun 2017 17:30:12 +0900

> 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,