If subscription to foreign table valid ?

Started by tusharover 8 years ago13 messages

tushar.ahuja@enterprisedb.com

over 8 years ago

Hi,

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options
(table_name 't1');
CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR: "t" is not a table
DETAIL: Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we
add subscription for it.

--
regards,tushar
EnterpriseDB https://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

robertmhaas@gmail.com

over 8 years ago

In reply to: tushar (#1)

Re: If subscription to foreign table valid ?

On Thu, May 11, 2017 at 8:25 AM, tushar <tushar.ahuja@enterprisedb.com> wrote:

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options
(table_name 't1');
CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR: "t" is not a table
DETAIL: Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we add
subscription for it.

This is not a complete test case, but it does appear to be odd behavior.

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

petr.jelinek@2ndquadrant.com

over 8 years ago

In reply to: tushar (#1)

Re: If subscription to foreign table valid ?

Hi,

On 11/05/17 14:25, tushar wrote:

Hi,

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options
(table_name 't1');
CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR: "t" is not a table
DETAIL: Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we
add subscription for it.

Thanks for report. What you can publish and what you can subscribe is
not necessarily same (we can write to relations which we can't capture
from wal, for example unlogged table can't be published but can be
subscribed).

However, the foreign tables indeed can't be subscribed. I originally
planned to have foreign tables allowed on subscriber but it turned out
to be more complex to implement than I had anticipated do I ripped the
code for that from the original patch.

We do check for this, but only during replication which we have to do
because the fact that relation 't' was foreign table during ALTER
SUBSCRIPTION does not mean that it won't be something else half hour later.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

I'll look into writing patch for this. I don't think it's beta blocker
though.

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

tushar.ahuja@enterprisedb.com

over 8 years ago

In reply to: Petr Jelinek (#3)

Re: If subscription to foreign table valid ?

On 05/11/2017 07:13 PM, Petr Jelinek wrote:

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

+1 , there are  few similar cases   - where user does not  get error at 
prompt , for instance
--when publication doesn't not exist
postgres=# create subscription sub connection 'dbname=postgres port=5000 
user=centos password=a' publication nowhere;
NOTICE:  synchronized table states
NOTICE:  created replication slot "sub" on publisher
CREATE SUBSCRIPTION
--No check validation for Publication name in ALTER
postgres=# alter subscription sub set publication _ refresh;
ALTER SUBSCRIPTION
--slot name given in ALTER
postgres=# alter subscription sub with ( slot name='nowhere');
ALTER SUBSCRIPTION
--and due to that , we are not able to drop it later.
postgres=# drop subscription sub;
ERROR:  could not drop the replication slot "nowhere" on publisher
DETAIL:  The error was: ERROR:  replication slot "nowhere" does not exist
postgres=#

--
regards,tushar
EnterpriseDB https://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

nehakhatri5@gmail.com

over 8 years ago

In reply to: Petr Jelinek (#3)

1 attachment(s)

Re: If subscription to foreign table valid ?

On 5/11/17, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:

Hi,

On 11/05/17 14:25, tushar wrote:

Hi,

I observed that -we cannot publish "foreign table" in Publication

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we
add subscription for it.

The above foreign table subscription succeeds only if the publisher
has published a same named normal table i.e.
create table t (n int);
CREATE PUBLICATION mypub FOR TABLE t;

I think in current implemetation of LogicalRep. it is users
responsibility to match the table definition at publisher and
subscriber. Subscriber can not determine by itself what publisher has
defined. This usecase does not align with this assumption.

However, the foreign tables indeed can't be subscribed.

I suspect that a user would want to subcribe a foreign table in real world.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

Yes, system should prohibit such operation though.
I tried to write to a patch to achieve this. It disalows subscription
of relation other than RELKIND_RELATION.
Attached is the patch. Comments?

Regards,
Neha

Attachments:

create_alter_sub_check_relkind.patchtext/x-patch; charset=US-ASCII; name=create_alter_sub_check_relkind.patchDownload

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index b76cdc5..f6013da 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -440,9 +440,20 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 			{
 				RangeVar   *rv = (RangeVar *) lfirst(lc);
 				Oid			relid;
+				Relation		relation;
 
 				relid = RangeVarGetRelid(rv, AccessShareLock, false);
 
+				relation = RelationIdGetRelation(relid);
+
+				if (RelationGetForm(relation)->relkind != RELKIND_RELATION)
+					ereport(ERROR,
+							(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+							 errmsg("\"%s\" is not a normal table",
+									RelationGetRelationName(relation)),
+							 errdetail("Only normal tables can be subscribed.")));
+				RelationClose(relation);
+
 				SetSubscriptionRelState(subid, relid, table_state,
 										InvalidXLogRecPtr);
 			}
@@ -553,8 +564,21 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 	{
 		RangeVar   *rv = (RangeVar *) lfirst(lc);
 		Oid			relid;
+		Relation		relation;
 
 		relid = RangeVarGetRelid(rv, AccessShareLock, false);
+
+		relation = RelationIdGetRelation(relid);
+
+		if (RelationGetForm(relation)->relkind != RELKIND_RELATION)
+			ereport(NOTICE,
+					(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+					 errmsg("\"%s\" is not a normal table",
+						RelationGetRelationName(relation)),
+					 errdetail("Only normal tables can be subscribed.")));
+
+		RelationClose(relation);
+
 		pubrel_local_oids[off++] = relid;
 
 		if (!bsearch(&relid, subrel_local_oids,

petr.jelinek@2ndquadrant.com

over 8 years ago

In reply to: Petr Jelinek (#3)

1 attachment(s)

Re: If subscription to foreign table valid ?

On 11/05/17 15:43, Petr Jelinek wrote:

Hi,

On 11/05/17 14:25, tushar wrote:

Hi,

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options
(table_name 't1');
CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR: "t" is not a table
DETAIL: Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we
add subscription for it.

Thanks for report. What you can publish and what you can subscribe is
not necessarily same (we can write to relations which we can't capture
from wal, for example unlogged table can't be published but can be
subscribed).

However, the foreign tables indeed can't be subscribed. I originally
planned to have foreign tables allowed on subscriber but it turned out
to be more complex to implement than I had anticipated do I ripped the
code for that from the original patch.

We do check for this, but only during replication which we have to do
because the fact that relation 't' was foreign table during ALTER
SUBSCRIPTION does not mean that it won't be something else half hour later.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

I'll look into writing patch for this. I don't think it's beta blocker
though.

So I moved the relkind check to single function and call it from all the
necessary places. See the attached

I now wonder if we should do some other checks as well (columns etc).

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

Attachments:

Check-relkind-of-tables-in-CREATE-ALTER-SUBSCRIPTION.patchbinary/octet-stream; name=Check-relkind-of-tables-in-CREATE-ALTER-SUBSCRIPTION.patchDownload

From a25ce7a17a4b2e814b74e5b6c845652498e6c2d3 Mon Sep 17 00:00:00 2001
From: Petr Jelinek <pjmodos@pjmodos.net>
Date: Thu, 11 May 2017 16:07:17 +0200
Subject: [PATCH] Check relkind of tables in CREATE/ALTER SUBSCRIPTION

We used to only check supported relkind on subscriber during the
replication, which is needed to ensure setup is valid and we don't
crash. However it's also useful to tell user immediately when CREATE or
ALTER SUBSCRIPTION is executed, that relation being added to
subscription is not of supported relkind.
---
 src/backend/commands/subscriptioncmds.c    | 11 +++++++++++
 src/backend/executor/execReplication.c     | 20 ++++++++++++++++++++
 src/backend/replication/logical/relation.c | 13 ++++---------
 src/include/executor/executor.h            |  2 ++
 4 files changed, 37 insertions(+), 9 deletions(-)

diff --git a/src/backend/commands/subscriptioncmds.c b/src/backend/commands/subscriptioncmds.c
index b76cdc5..49c2cef 100644
--- a/src/backend/commands/subscriptioncmds.c
+++ b/src/backend/commands/subscriptioncmds.c
@@ -33,6 +33,8 @@
 #include "commands/event_trigger.h"
 #include "commands/subscriptioncmds.h"
 
+#include "executor/executor.h"
+
 #include "nodes/makefuncs.h"
 
 #include "replication/logicallauncher.h"
@@ -443,6 +445,10 @@ CreateSubscription(CreateSubscriptionStmt *stmt, bool isTopLevel)
 
 				relid = RangeVarGetRelid(rv, AccessShareLock, false);
 
+				/* Check for supported relkind. */
+				CheckSubscriptionRelkind(get_rel_relkind(relid),
+										 rv->schemaname, rv->relname);
+
 				SetSubscriptionRelState(subid, relid, table_state,
 										InvalidXLogRecPtr);
 			}
@@ -555,6 +561,11 @@ AlterSubscription_refresh(Subscription *sub, bool copy_data)
 		Oid			relid;
 
 		relid = RangeVarGetRelid(rv, AccessShareLock, false);
+
+		/* Check for supported relkind. */
+		CheckSubscriptionRelkind(get_rel_relkind(relid),
+								 rv->schemaname, rv->relname);
+
 		pubrel_local_oids[off++] = relid;
 
 		if (!bsearch(&relid, subrel_local_oids,
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 327a0ba..6af8018 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -551,3 +551,23 @@ CheckCmdReplicaIdentity(Relation rel, CmdType cmd)
 						RelationGetRelationName(rel)),
 				 errhint("To enable deleting from the table, set REPLICA IDENTITY using ALTER TABLE.")));
 }
+
+
+/*
+ * Check if we support writing into specific relkind.
+ *
+ * The nspname and relname are only needed for error reporting.
+ */
+void
+CheckSubscriptionRelkind(char relkind, const char *nspname,
+						 const char *relname)
+{
+	/*
+	 * We currently only support writing to regular tables.
+	 */
+	if (relkind != RELKIND_RELATION)
+		ereport(ERROR,
+				(errcode(ERRCODE_WRONG_OBJECT_TYPE),
+				 errmsg("logical replication target relation \"%s.%s\" is not a table",
+						nspname, relname)));
+}
diff --git a/src/backend/replication/logical/relation.c b/src/backend/replication/logical/relation.c
index 7c93bfb..590355a 100644
--- a/src/backend/replication/logical/relation.c
+++ b/src/backend/replication/logical/relation.c
@@ -20,6 +20,7 @@
 #include "access/sysattr.h"
 #include "catalog/namespace.h"
 #include "catalog/pg_subscription_rel.h"
+#include "executor/executor.h"
 #include "nodes/makefuncs.h"
 #include "replication/logicalrelation.h"
 #include "replication/worker_internal.h"
@@ -258,15 +259,9 @@ logicalrep_rel_open(LogicalRepRelId remoteid, LOCKMODE lockmode)
 							remoterel->nspname, remoterel->relname)));
 		entry->localrel = heap_open(relid, NoLock);
 
-		/*
-		 * We currently only support writing to regular and partitioned
-		 * tables.
-		 */
-		if (entry->localrel->rd_rel->relkind != RELKIND_RELATION)
-			ereport(ERROR,
-					(errcode(ERRCODE_WRONG_OBJECT_TYPE),
-					 errmsg("logical replication target relation \"%s.%s\" is not a table",
-							remoterel->nspname, remoterel->relname)));
+		/* Check for supported relkind. */
+		CheckSubscriptionRelkind(entry->localrel->rd_rel->relkind,
+								 remoterel->nspname, remoterel->relname);
 
 		/*
 		 * Build the mapping of local attribute numbers to remote attribute
diff --git a/src/include/executor/executor.h b/src/include/executor/executor.h
index 3107cf5..daae7aa 100644
--- a/src/include/executor/executor.h
+++ b/src/include/executor/executor.h
@@ -534,5 +534,7 @@ extern void ExecSimpleRelationDelete(EState *estate, EPQState *epqstate,
 						 TupleTableSlot *searchslot);
 extern void CheckCmdReplicaIdentity(Relation rel, CmdType cmd);
 
+extern void CheckSubscriptionRelkind(char relkind, const char *nspname,
+									 const char *relname);
 
 #endif   /* EXECUTOR_H  */
-- 
2.7.4

nehakhatri5@gmail.com

over 8 years ago

In reply to: Neha Khatri (#5)

Re: If subscription to foreign table valid ?

[Correction below]

On 5/12/17, Neha Khatri <nehakhatri5@gmail.com> wrote:

On 5/11/17, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote

However, the foreign tables indeed can't be subscribed.

Yes, I suspect that a user would _not_ want to subcribe a foreign
table in real world.

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

petr.jelinek@2ndquadrant.com

over 8 years ago

In reply to: Neha Khatri (#5)

Re: If subscription to foreign table valid ?

On 12/05/17 15:55, Neha Khatri wrote:

On 5/11/17, Petr Jelinek <petr.jelinek@2ndquadrant.com> wrote:

Hi,

On 11/05/17 14:25, tushar wrote:

Hi,

I observed that -we cannot publish "foreign table" in Publication

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we
add subscription for it.

The above foreign table subscription succeeds only if the publisher
has published a same named normal table i.e.
create table t (n int);
CREATE PUBLICATION mypub FOR TABLE t;

I think in current implemetation of LogicalRep. it is users
responsibility to match the table definition at publisher and
subscriber. Subscriber can not determine by itself what publisher has
defined. This usecase does not align with this assumption.

However, the foreign tables indeed can't be subscribed.

I suspect that a user would want to subcribe a foreign table in real world.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

Yes, system should prohibit such operation though.
I tried to write to a patch to achieve this. It disalows subscription
of relation other than RELKIND_RELATION.
Attached is the patch. Comments?

I sent my version of patch in parallel. I think we don't need to do the
relation open like you did, all the info is in syscache.

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

nehakhatri5@gmail.com

over 8 years ago

In reply to: Petr Jelinek (#8)

Re: If subscription to foreign table valid ?

I sent my version of patch in parallel. I think we don't need to do the
relation open like you did, all the info is in syscache.

That's right.

Regards,
Neha

noah@leadboat.com

over 8 years ago

In reply to: tushar (#1)

Re: If subscription to foreign table valid ?

On Thu, May 11, 2017 at 05:55:02PM +0530, tushar wrote:

I observed that -we cannot publish "foreign table" in Publication

postgres=# create foreign table t (n int) server db1_server options
(table_name 't1');
CREATE FOREIGN TABLE

postgres=# create publication pub for table t;
ERROR: "t" is not a table
DETAIL: Only tables can be added to publications.
postgres=#

but same thing is not true for Subscription

postgres=# create foreign table t (n int) server db1_server options
(table_name 't');
CREATE FOREIGN TABLE
postgres=# alter subscription sub refresh publication ;
NOTICE: added subscription for table public.t
ALTER SUBSCRIPTION

Is this an expected behavior ? if we cannot publish then how can we add
subscription for it.

[Action required within three days. This is a generic notification.]

The above-described topic is currently a PostgreSQL 10 open item. Peter,
since you committed the patch believed to have created it, you own this open
item. If some other commit is more relevant or if this does not belong as a
v10 open item, please let us know. Otherwise, please observe the policy on
open item ownership[1]/messages/by-id/20170404140717.GA2675809@tornado.leadboat.com and send a status update within three calendar days of
this message. Include a date for your subsequent status update. Testers may
discover new open items at any time, and I want to plan to get them all fixed
well in advance of shipping v10. Consequently, I will appreciate your efforts
toward speedy resolution. Thanks.

[1]: /messages/by-id/20170404140717.GA2675809@tornado.leadboat.com

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

nehakhatri5@gmail.com

over 8 years ago

In reply to: Petr Jelinek (#6)

Re: If subscription to foreign table valid ?

On Fri, May 12, 2017 at 11:58 PM, Petr Jelinek <petr.jelinek@2ndquadrant.com

wrote:

On 11/05/17 15:43, Petr Jelinek wrote:

Hi,

We do check for this, but only during replication which we have to do
because the fact that relation 't' was foreign table during ALTER
SUBSCRIPTION does not mean that it won't be something else half hour

later.

I think it does make sense to add check for this into CREATE/ALTER
SUBSCRIBER though so that user is informed immediately about the mistake
rather than by errors in the logs later.

I'll look into writing patch for this. I don't think it's beta blocker
though.

So I moved the relkind check to single function and call it from all the
necessary places. See the attached

With this patch the error will be like this:

logical replication target relation public.t is not a table

But it is possible that the referred table is Foreign Table of Partitioned
table (so actually the referred object is indeed a table).
Would it make sense to specify in the message that the table is not a
normal table or something in that line?

Regards,
Neha

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Petr Jelinek (#6)

Re: If subscription to foreign table valid ?

On 5/12/17 09:58, Petr Jelinek wrote:

So I moved the relkind check to single function and call it from all the
necessary places. See the attached

committed

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

Peter Eisentraut

peter.eisentraut@2ndquadrant.com

over 8 years ago

In reply to: Neha Khatri (#11)

Re: If subscription to foreign table valid ?

On 5/14/17 23:55, Neha Khatri wrote:

With this patch the error will be like this:

logical replication target relation public.t is not a table

But it is possible that the referred table is Foreign Table of
Partitioned table (so actually the referred object is indeed a table).
Would it make sense to specify in the message that the table is not a
normal table or something in that line?

This is how we phrase the message in other places where we check the
relkind. We should keep that consistent.

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