BUG #14785: Logical replication does not work after adding a column. Bug?
The following bug has been logged on the website:
Bug reference: 14785
Logged by: yxq
Email address: yxq@o2.pl
PostgreSQL version: 10beta3
Operating system: Linux Debian Stretch 64-bit
Description:
Server Master:
port = 5432
wal_level = logical
Server Slave:
port = 5433
wal_level = logical
***** Steps to reproduce:
* Master:
CREATE TABLE users
(
id SERIAL PRIMARY KEY,
firstname VARCHAR(100) NOT NULL,
lastname VARCHAR(100) NOT NULL
);
INSERT INTO users(firstname,lastname) VALUES('fu1','lu1');
INSERT INTO users(firstname,lastname) VALUES('fu2','lu2');
SELECT * FROM users;
result:
id | firstname | lastname
----+-----------+----------
1 | fu1 | lu1
2 | fu2 | lu2
(2 rows)
CREATE PUBLICATION mypub FOR ALL TABLES;
*** Slave:
CREATE TABLE users
(
id SERIAL PRIMARY KEY,
firstname VARCHAR(100) NOT NULL,
lastname VARCHAR(100) NOT NULL
);
CREATE SUBSCRIPTION mysub
CONNECTION 'port=5432 dbname=postgres'
PUBLICATION mypub;
SELECT * FROM users;
result:
id | firstname | lastname
----+-----------+----------
1 | fu1 | lu1
2 | fu2 | lu2
(2 rows)
*** Master:
INSERT INTO users(firstname,lastname) VALUES('fu3','lu3');
SELECT * FROM users;
result:
id | firstname | lastname
----+-----------+----------
1 | fu1 | lu1
2 | fu2 | lu2
3 | fu3 | lu3
(3 rows)
*** Slave:
SELECT * FROM users;
result:
id | firstname | lastname
----+-----------+----------
1 | fu1 | lu1
2 | fu2 | lu2
3 | fu3 | lu3
(3 rows)
ALTER TABLE users ADD enabled BOOLEAN NOT NULL DEFAULT true;
*** Master:
ALTER TABLE users ADD enabled BOOLEAN NOT NULL DEFAULT true;
INSERT INTO users(firstname,lastname) VALUES('fu4','lu4');
SELECT * FROM users;
result:
id | firstname | lastname | enabled
----+-----------+----------+---------
1 | fu1 | lu1 | t
2 | fu2 | lu2 | t
3 | fu3 | lu3 | t
4 | fu4 | lu4 | t
(4 rows)
*** Slave:
SELECT * FROM users;
result:
id | firstname | lastname | enabled
----+-----------+----------+---------
1 | fu1 | lu1 | t
2 | fu2 | lu2 | t
3 | fu3 | lu3 | t
(3 rows)
*** Slave Log:
2017-08-20 18:25:59.430 UTC [4764] LOG: database system is ready to accept
connections
2017-08-20 18:27:09.938 UTC [4781] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:27:09.947 UTC [4783] LOG: logical replication table
synchronization worker for subscription "mysub", table "users" has started
2017-08-20 18:27:10.354 UTC [4783] LOG: logical replication table
synchronization worker for subscription "mysub", table "users" has
finished
2017-08-20 18:42:46.121 UTC [4781] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:42:46.122 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4781) exited with exit code 1
2017-08-20 18:42:46.124 UTC [4837] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:42:46.154 UTC [4837] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:42:46.155 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4837) exited with exit code 1
2017-08-20 18:42:51.163 UTC [4839] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:42:51.221 UTC [4839] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:42:51.223 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4839) exited with exit code 1
2017-08-20 18:42:56.234 UTC [4841] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:42:56.378 UTC [4841] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:42:56.380 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4841) exited with exit code 1
2017-08-20 18:43:01.390 UTC [4845] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:43:01.568 UTC [4845] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:43:01.570 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4845) exited with exit code 1
2017-08-20 18:43:06.580 UTC [4847] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:43:06.726 UTC [4847] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:43:06.729 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4847) exited with exit code 1
2017-08-20 18:43:11.739 UTC [4849] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:43:11.906 UTC [4849] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:43:11.908 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4849) exited with exit code 1
2017-08-20 18:43:16.918 UTC [4851] LOG: logical replication apply worker
for subscription "mysub" has started
2017-08-20 18:43:17.077 UTC [4851] ERROR: logical replication target
relation "public.pg_temp_16386" does not exist
2017-08-20 18:43:17.079 UTC [4764] LOG: worker process: logical replication
worker for subscription 16392 (PID 4851) exited with exit code 1
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
When you add a column on the publication side, you also need to add it
on the subscription side, otherwise there is nowhere to put the data.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Hi,
On 2017-08-22 15:22:41 -0400, Peter Eisentraut wrote:
When you add a column on the publication side, you also need to add it
on the subscription side, otherwise there is nowhere to put the data.
Op's sql shows that that's done. The problem is the table rewrite not
being handled nicely by logical decoding / replication.
- Andres
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Dnia 22 sierpnia 2017 21:34 Andres Freund <andres@anarazel.de> napisał(a): Hi, On 2017-08-22 15:22:41 -0400, Peter Eisentraut wrote: When you add a column on the publication side, you also need to add it on the subscription side, otherwise there is nowhere to put the data. Op's sql shows that that's done. The problem is the table rewrite not being handled nicely by logical decoding / replication. - Andres Yes, I added the column first to the Slave, then added to the Master. I also checked the reverse case: I added the column first to the Master, then added to the Slave. The problem is the same. yxq
On 8/22/17 15:34, Andres Freund wrote:
On 2017-08-22 15:22:41 -0400, Peter Eisentraut wrote:
When you add a column on the publication side, you also need to add it
on the subscription side, otherwise there is nowhere to put the data.Op's sql shows that that's done. The problem is the table rewrite not
being handled nicely by logical decoding / replication.
OK, I see it now.
The problem happens on the publisher side. After the table rewrite on
the publisher side, the output plugin starts sending the wrong relname.
The name comes straight from RelationGetRelationName() (in
logicalrep_write_rel()) with the Relation that gets passed into the
pgoutput_change() callback, so there doesn't appear to be an obvious
logic error in the output plugin.
Any ideas? Do we need to renew a snapshot somehow, perhaps?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 2017-08-23 09:25:13 -0400, Peter Eisentraut wrote:
On 8/22/17 15:34, Andres Freund wrote:
On 2017-08-22 15:22:41 -0400, Peter Eisentraut wrote:
When you add a column on the publication side, you also need to add it
on the subscription side, otherwise there is nowhere to put the data.Op's sql shows that that's done. The problem is the table rewrite not
being handled nicely by logical decoding / replication.OK, I see it now.
The problem happens on the publisher side. After the table rewrite on
the publisher side, the output plugin starts sending the wrong relname.The name comes straight from RelationGetRelationName() (in
logicalrep_write_rel()) with the Relation that gets passed into the
pgoutput_change() callback, so there doesn't appear to be an obvious
logic error in the output plugin.Any ideas? Do we need to renew a snapshot somehow, perhaps?
This is "known" behaviour - this is the actual data WAL logged :(. Table
rewrites generate these pg_temp* tables and log the data there...
Greetings,
Andres Freund
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 8/23/17 12:31, Andres Freund wrote:
This is "known" behaviour - this is the actual data WAL logged :(. Table
rewrites generate these pg_temp* tables and log the data there...
Hmm, I see.
Possibly, one way a user could recover from this is to add the column on
the subscriber, rename to table on the subscriber to the temp name, then
wait until all the changes from the rewrite are applied, at which point
it should start complaining in the logs that the original table name
does not exist, then rename the table back.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 2017-08-23 17:35:11 -0400, Peter Eisentraut wrote:
On 8/23/17 12:31, Andres Freund wrote:
This is "known" behaviour - this is the actual data WAL logged :(. Table
rewrites generate these pg_temp* tables and log the data there...Hmm, I see.
Possibly, one way a user could recover from this is to add the column on
the subscriber, rename to table on the subscriber to the temp name, then
wait until all the changes from the rewrite are applied, at which point
it should start complaining in the logs that the original table name
does not exist, then rename the table back.
I think we could actually kind of solve this by just ignoring pg_temp*
tables in the output plugin. Given we don't do DDL replication at this
point, that seems good enough. "all" we need is a way to make sure we're
not confusing the pg_temp* tables with a table the user has declared as
pg_temp - but we could check subscription state for that?
Greetings,
Andres Freund
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 8/23/17 17:39, Andres Freund wrote:
I think we could actually kind of solve this by just ignoring pg_temp*
tables in the output plugin. Given we don't do DDL replication at this
point, that seems good enough. "all" we need is a way to make sure we're
not confusing the pg_temp* tables with a table the user has declared as
pg_temp - but we could check subscription state for that?
How about this patch? There is some heuristic element in there, but it
seems better than failing as we currently do.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Attachments:
0001-Handle-heap-rewrites-better-in-logical-replication.patchtext/plain; charset=UTF-8; name=0001-Handle-heap-rewrites-better-in-logical-replication.patch; x-mac-creator=0; x-mac-type=0Download+91-1
On 9/13/17 09:46, Peter Eisentraut wrote:
On 8/23/17 17:39, Andres Freund wrote:
I think we could actually kind of solve this by just ignoring pg_temp*
tables in the output plugin. Given we don't do DDL replication at this
point, that seems good enough. "all" we need is a way to make sure we're
not confusing the pg_temp* tables with a table the user has declared as
pg_temp - but we could check subscription state for that?How about this patch? There is some heuristic element in there, but it
seems better than failing as we currently do.
Thoughts on this patch? Might be good to include in PG10.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
/messages/by-id/attachment/55041/0001-Handle-heap-rewrites-better-in-logical-replication.patch
This fix solves the problem. It would be nice if this or another, better
patch in pg10 was implemented.
yxq
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Import Notes
Resolved by subject fallback
On 9/24/17 16:54, yxq wrote:
/messages/by-id/attachment/55041/0001-Handle-heap-rewrites-better-in-logical-replication.patch
This fix solves the problem. It would be nice if this or another, better
patch in pg10 was implemented.
I would like to commit my patch to PG10, but I'm hoping for an opinion
from another developer.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 2017-09-25 12:14:18 -0400, Peter Eisentraut wrote:
On 9/24/17 16:54, yxq wrote:
/messages/by-id/attachment/55041/0001-Handle-heap-rewrites-better-in-logical-replication.patch
This fix solves the problem. It would be nice if this or another, better
patch in pg10 was implemented.I would like to commit my patch to PG10, but I'm hoping for an opinion
from another developer.
I'll have a look.
Greetings,
Andres Freund
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 25/09/17 18:14, Peter Eisentraut wrote:
On 9/24/17 16:54, yxq wrote:
/messages/by-id/attachment/55041/0001-Handle-heap-rewrites-better-in-logical-replication.patch
This fix solves the problem. It would be nice if this or another, better
patch in pg10 was implemented.I would like to commit my patch to PG10, but I'm hoping for an opinion
from another developer.
From my perspective, this is what we should do in PG10. It's similar to
how pglogical handles this situation. Longer term it would be nice if
there was more sane way of identifying and handling the rewrites but
that's not something we can solve at RC time.
--
Petr Jelinek http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Hi,
On 2017-09-13 09:46:43 -0400, Peter Eisentraut wrote:
The prevent this problem, we filter out any tables that match this
naming pattern and match an actual table from FOR ALL TABLES
publications. This is only a heuristic, meaning that user tables that
match that naming could accidentally be omitted. A more robust solution
might require an explicit marking of such tables in pg_class somehow.
Yes, I think that makes sense.
+ /* + * Skip tables that look like they are from a heap rewrite (see + * make_new_heap()). We need to skip them because the subscriber + * won't have a table by that name to receive the data. That + * means we won't ship the new data in, say, an added column with + * a DEFAULT, but if the user applies the same DDL manually on the + * subscriber, then this will work out for them. + * + * We only need to consider the alltables case, because such a + * transient heap won't be an explicit member of a publication. + */ + if (pub->alltables) + { + char *relname = get_rel_name(relid); + unsigned int u; + + if (sscanf(relname, "pg_temp_%u", &u) == 1) + { + if (get_rel_relkind(u) == RELKIND_RELATION) + break; + } + } +
This'll accept tablenames like pg_temp_1foo, right? Might be worth
being a bit narrower in the test.
Greetings,
Andres Freund
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
On 9/25/17 15:16, Andres Freund wrote:
This'll accept tablenames like pg_temp_1foo, right? Might be worth
being a bit narrower in the test.
Committed with that change. Thanks.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 9/25/17 15:16, Andres Freund wrote:
This'll accept tablenames like pg_temp_1foo, right? Might be worth
being a bit narrower in the test.
Committed with that change. Thanks.
This patch is using the wrong approach entirely. Every other place in
the backend that is trying to exclude temp relations uses a test on the
containing namespace, not the relname. You should be using
RELATION_IS_OTHER_TEMP(), or if there's good reason to decide this
without opening the relation, maybe you could get away with
isOtherTempNamespace(get_rel_namespace(relid)).
I also note that as committed, the patch will dump core on a
concurrently-dropped relation, because get_rel_name returns NULL
under such circumstances.
BTW, get_rel_sync_entry has some other serious problems: it is not being
at all careful about whether persistent data structures are left in sane
states if it gets an error partway through. In particular it'll leave
behind a new hash entry in entirely-unknown state, and if LoadPublications
gets an error, it will also leave a time bomb behind in the form of
not nil, but already-list-freed, data->publications. And I sure do not
understand why a single static variable publications_valid is being used
to remember validity of data->publications ... couldn't there be more
than one of those?
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs
Tom,
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 9/25/17 15:16, Andres Freund wrote:
This'll accept tablenames like pg_temp_1foo, right? Might be worth
being a bit narrower in the test.Committed with that change. Thanks.
This patch is using the wrong approach entirely. Every other place in
the backend that is trying to exclude temp relations uses a test on the
containing namespace, not the relname.
The specific issue here is that the new pg_class entry is created in the
same namespace, not in the temp one. The commit mentions make_new_heap()
specifically because that's where the issue is coming from because
that's creating this new pg_temp_XXX table in the regular user
namespace.
I'm not a huge fan of this approach either, really, but I'm not sure
that there's a better answer either.
I also note that as committed, the patch will dump core on a
concurrently-dropped relation, because get_rel_name returns NULL
under such circumstances.
That's certainly no good and should be checked for.
BTW, get_rel_sync_entry has some other serious problems: it is not being
at all careful about whether persistent data structures are left in sane
states if it gets an error partway through. In particular it'll leave
behind a new hash entry in entirely-unknown state, and if LoadPublications
gets an error, it will also leave a time bomb behind in the form of
not nil, but already-list-freed, data->publications. And I sure do not
understand why a single static variable publications_valid is being used
to remember validity of data->publications ... couldn't there be more
than one of those?
This all certainly doesn't sound good, but I'm not as familiar with
these bits.
Thanks!
Stephen
Tom,
* Stephen Frost (sfrost@snowman.net) wrote:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> writes:
On 9/25/17 15:16, Andres Freund wrote:
This'll accept tablenames like pg_temp_1foo, right? Might be worth
being a bit narrower in the test.Committed with that change. Thanks.
This patch is using the wrong approach entirely. Every other place in
the backend that is trying to exclude temp relations uses a test on the
containing namespace, not the relname.The specific issue here is that the new pg_class entry is created in the
same namespace, not in the temp one. The commit mentions make_new_heap()
specifically because that's where the issue is coming from because
that's creating this new pg_temp_XXX table in the regular user
namespace.
And, to be clear, the new table also doesn't necessairly have
RELPERSISTENCE_TEMP either.
Really is pretty ugly how make_new_heap() operates, it seems...
Thanks!
Stephen
Stephen Frost <sfrost@snowman.net> writes:
* Tom Lane (tgl@sss.pgh.pa.us) wrote:
This patch is using the wrong approach entirely. Every other place in
the backend that is trying to exclude temp relations uses a test on the
containing namespace, not the relname.
The specific issue here is that the new pg_class entry is created in the
same namespace, not in the temp one. The commit mentions make_new_heap()
specifically because that's where the issue is coming from because
that's creating this new pg_temp_XXX table in the regular user
namespace.
Um ... but that transient relation should never be visible to any other
backend, because we delete it before committing. If there is someplace
where this is not the case, then we need to change that, because it'll
break everything else that scans pg_class, such as VACUUM.
In short: this code is unlike every other similar check in the backend.
I find it unlikely that this is right and all the others are wrong.
regards, tom lane
--
Sent via pgsql-bugs mailing list (pgsql-bugs@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-bugs