Re: [HACKERS] Different table schema in logical replication crashes

[Peter Eisentraut]

On 4/17/17 08:47, Euler Taveira wrote:
> Patch works fine. However, I don't see any documentation about
> supporting different schemas for logical replication. Is it an oversight?

I have added more documentation about that.

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

Re: [HACKERS] Different table schema in logical replication crashes

[Peter Eisentraut]

On 4/14/17 21:36, Petr Jelinek wrote:
> I tried something bit different which seems cleaner to me - use the
> pstate->r_table instead of ad-hock locally made up range table and fill
> that using standard addRangeTableEntryForRelation. Both in tablesync and
> in DoCopy instead of the old coding.

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

Re: [HACKERS] Different table schema in logical replication crashes

[Euler Taveira]

2017-04-14 22:36 GMT-03:00 Petr Jelinek :

> I tried something bit different which seems cleaner to me - use the
> pstate->r_table instead of ad-hock locally made up range table and fill
> that using standard addRangeTableEntryForRelation. Both in tablesync and
> in DoCopy instead of the old coding.
>

Patch works fine. However, I don't see any documentation about supporting
different schemas for logical replication. Is it an oversight?


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento

Re: [HACKERS] Different table schema in logical replication crashes

[Petr Jelinek]

On 14/04/17 17:33, Peter Eisentraut wrote:
> On 4/14/17 08:49, Petr Jelinek wrote:
>>> Are we prepared to support different schemas in v10? Or should we
>>> disallow it for v10 and add a TODO?
>>>
>>
>> Ah nuts, yes it's supposed to be supported, we seem to not initialize
>> cstate->range_table in tablesync which causes this bug. The CopyState
>> struct is private to copy.c so we can't easily set cstate->range_table
>> externally. I wonder if tablesync should just construct CopyStmt instead
>> of calling the lower level API.
> 
> Maybe pass the range_table to BeginCopyFrom so that it can write it into
> cstate?
> 

I tried something bit different which seems cleaner to me - use the
pstate->r_table instead of ad-hock locally made up range table and fill
that using standard addRangeTableEntryForRelation. Both in tablesync and
in DoCopy instead of the old coding.

-- 
  Petr Jelinek  http://www.2ndQuadrant.com/
  PostgreSQL Development, 24x7 Support, Training & Services
From 359c10959f639c1dd3b01f90d459ee4f03ccbd0b Mon Sep 17 00:00:00 2001
From: Petr Jelinek 
Date: Sat, 15 Apr 2017 03:27:32 +0200
Subject: [PATCH] Set range table for CopyFrom in tablesync

This changes the mechanism of how range table is passed to CopyFrom
executor state. We used to generate the range table and one entry for
the relation manually inside DoCopy.Now we use
addRangeTableEntryForRelation to setup the range table and relation
entry for the ParseState which is then passed down by BeginCopyFrom.
---
 src/backend/commands/copy.c | 17 +++--
 src/backend/replication/logical/tablesync.c | 13 +++--
 2 files changed, 18 insertions(+), 12 deletions(-)

diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index b5af2be..a786d7b 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -34,6 +34,7 @@
 #include "libpq/pqformat.h"
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
+#include "parser/parse_relation.h"
 #include "optimizer/clauses.h"
 #include "optimizer/planner.h"
 #include "nodes/makefuncs.h"
@@ -787,7 +788,6 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
Relationrel;
Oid relid;
RawStmt*query = NULL;
-   List   *range_table = NIL;
 
/* Disallow COPY to/from file or program except to superusers. */
if (!pipe && !superuser())
@@ -809,7 +809,6 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
if (stmt->relation)
{
TupleDesc   tupDesc;
-   AclMode required_access = (is_from ? ACL_INSERT : 
ACL_SELECT);
List   *attnums;
ListCell   *cur;
RangeTblEntry *rte;
@@ -822,12 +821,8 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
relid = RelationGetRelid(rel);
 
-   rte = makeNode(RangeTblEntry);
-   rte->rtekind = RTE_RELATION;
-   rte->relid = RelationGetRelid(rel);
-   rte->relkind = rel->rd_rel->relkind;
-   rte->requiredPerms = required_access;
-   range_table = list_make1(rte);
+   rte = addRangeTableEntryForRelation(pstate, rel, NULL, false, 
false);
+   rte->requiredPerms = (stmt->is_from ? ACL_INSERT : ACL_SELECT);
 
tupDesc = RelationGetDescr(rel);
attnums = CopyGetAttnums(tupDesc, rel, stmt->attlist);
@@ -841,7 +836,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
else
rte->selectedCols = 
bms_add_member(rte->selectedCols, attno);
}
-   ExecCheckRTPerms(range_table, true);
+   ExecCheckRTPerms(pstate->p_rtable, true);
 
/*
 * Permission check for row security policies.
@@ -977,7 +972,6 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 
cstate = BeginCopyFrom(pstate, rel, stmt->filename, 
stmt->is_program,
   NULL, stmt->attlist, 
stmt->options);
-   cstate->range_table = range_table;
*processed = CopyFrom(cstate);  /* copy from file to database */
EndCopyFrom(cstate);
}
@@ -2921,6 +2915,9 @@ BeginCopyFrom(ParseState *pstate,
cstate->raw_buf = (char *) palloc(RAW_BUF_SIZE + 1);
cstate->raw_buf_index = cstate->raw_buf_len = 0;
 
+   /* Assign range table, we'll need it in CopyFrom. */
+   cstate->range_table = pstate->p_rtable;
+
tupDesc = RelationGetDescr(cstate->rel);
attr = tupDesc->attrs;
num_phys_attrs = tupDesc->natts;
diff --git a/src/backend/replication/logical/tablesync.c 
b/src/backend/replication/logical/tablesync.c
index d1f2734..9ba2c01 100644
--- a/src/backend/replication/logical/tablesync.c
+++ b/src/backend/replication/logical/tablesync.c
@@ -93,6 +93,8 @@
 
 #include "commands/copy.h"
 
+#

Re: [HACKERS] Different table schema in logical replication crashes

[Petr Jelinek]

On 14/04/17 17:33, Peter Eisentraut wrote:
> On 4/14/17 08:49, Petr Jelinek wrote:
>>> Are we prepared to support different schemas in v10? Or should we
>>> disallow it for v10 and add a TODO?
>>>
>>
>> Ah nuts, yes it's supposed to be supported, we seem to not initialize
>> cstate->range_table in tablesync which causes this bug. The CopyState
>> struct is private to copy.c so we can't easily set cstate->range_table
>> externally. I wonder if tablesync should just construct CopyStmt instead
>> of calling the lower level API.
> 
> Maybe pass the range_table to BeginCopyFrom so that it can write it into
> cstate?
> 

That would work. The reason why I am thinking of creating CopyStmt
instead is that to create the range_table, we'll basically have to
duplicate the code from DoCopy verbatim. Obviously making CopyStmt isn't
without troubles either as it would have to newly support the callback
input.

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

Re: [HACKERS] Different table schema in logical replication crashes

[Peter Eisentraut]

On 4/14/17 08:49, Petr Jelinek wrote:
>> Are we prepared to support different schemas in v10? Or should we
>> disallow it for v10 and add a TODO?
>>
> 
> Ah nuts, yes it's supposed to be supported, we seem to not initialize
> cstate->range_table in tablesync which causes this bug. The CopyState
> struct is private to copy.c so we can't easily set cstate->range_table
> externally. I wonder if tablesync should just construct CopyStmt instead
> of calling the lower level API.

Maybe pass the range_table to BeginCopyFrom so that it can write it into
cstate?

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

Re: [HACKERS] Different table schema in logical replication crashes

[Petr Jelinek]

On 13/04/17 05:04, Euler Taveira wrote:
> Hi,
> 
> If a certain table has different schemas and the subscriber table has an
> unmatched column with a not null constraint, the logical replication
> crashes with the above stack trace.
> 
> [snip]
> 
> Are we prepared to support different schemas in v10? Or should we
> disallow it for v10 and add a TODO?
> 

Ah nuts, yes it's supposed to be supported, we seem to not initialize
cstate->range_table in tablesync which causes this bug. The CopyState
struct is private to copy.c so we can't easily set cstate->range_table
externally. I wonder if tablesync should just construct CopyStmt instead
of calling the lower level API.

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

[HACKERS] Different table schema in logical replication crashes

[Euler Taveira]

Hi,

If a certain table has different schemas and the subscriber table has an
unmatched column with a not null constraint, the logical replication
crashes with the above stack trace.

-- publisher
CREATE TABLE test (a integer, b varchar not null, c numeric not null,
PRIMARY KEY(a));
-- subscriber
CREATE TABLE test (a integer, b varchar not null, c numeric not null, d
integer not null, PRIMARY KEY(a));

Program terminated with signal SIGSEGV, Segmentation fault.
#0  list_nth_cell (n=0, list=0x0) at list.c:411
411{
(gdb) bt
#0  list_nth_cell (n=0, list=0x0) at list.c:411
#1  list_nth (list=0x0, n=0) at list.c:413
#2  0x005ddc6b in ExecConstraints
(resultRelInfo=resultRelInfo@entry=0xf96868,
slot=slot@entry=0xf984d8, estate=estate@entry=0xfc3808) at execMain.c:1881
#3  0x0057b0ba in CopyFrom (cstate=0xf980c8) at copy.c:2652
#4  0x006ae3bb in copy_table (rel=) at
tablesync.c:682
#5  LogicalRepSyncTableStart (origin_startpos=0x7ffe9c340640) at
tablesync.c:789
#6  0x006afb0f in ApplyWorkerMain (main_arg=) at
worker.c:1521
#7  0x00684813 in StartBackgroundWorker () at bgworker.c:838
#8  0x0068f6a2 in do_start_bgworker (rw=0xf0cbb0) at
postmaster.c:5577
#9  maybe_start_bgworker () at postmaster.c:5761
#10 0x00690195 in sigusr1_handler (postgres_signal_arg=) at postmaster.c:5015
#11 
#12 0x7fcd075f6873 in __select_nocancel () at
../sysdeps/unix/syscall-template.S:81
#13 0x00476c0c in ServerLoop () at postmaster.c:1693
#14 0x00691342 in PostmasterMain (argc=argc@entry=1,
argv=argv@entry=0xee4eb0)
at postmaster.c:1337
#15 0x00478684 in main (argc=1, argv=0xee4eb0) at main.c:228

Are we prepared to support different schemas in v10? Or should we disallow
it for v10 and add a TODO?


-- 
   Euler Taveira   Timbira -
http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento