Thread: logical changeset generation v6

logical changeset generation v6

From

Andres Freund

Date:

14 September 2013, 23:50:24

Hi!

Attached you can find the newest version of the logical changeset
generation patchset. Reduced by a couple of patches because the have
been committed last round. Hurray! and thanks.

The explanation of how to use the patch from last time:
http://archives.postgresql.org/message-id/20130614224817.GA19641%40awork2.anarazel.de
still holds true, so I am not going to repeat it here.

The individual patches are:
0001 wal_decoding: Allow walsender's to connect to a specific database
One logical decoding operation can only decode content from one
database at a time. Because of that the walsender needs to connect
to a specific database. The earlier "replication=on/off" parameter
now also has a valid parameter "database" which allows that.

0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
Imo relatively unproblematic and even useful without changeset extraction.

0003 wal_decoding: Add information about a tables primary key to struct RelationData
Not much comments on this in the past. Kevin thinks we might want to
choose the best candidate key in a more elaborate manner.

0004 wal_decoding: Introduce wal decoding via catalog timetravel
The actual feature. Got cleaned up and shrunk since the last submission.

0005 wal_decoding: test_decoding: Add a simple decoding module in contrib
Example output plugin that's also used for testing.

0006 wal_decoding: pg_receivellog: Introduce pg_receivexlog equivalent for logical changes
Commandline utility to receive the changestream and manipulate slots.

0007 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
Allows to not only create and destroy logical slots which is part of
0005, but also receive the changestream via an SQL SRF.

Re: logical changeset generation v6

From

Fujii Masao

Date:

19 September 2013, 08:08:42

On Tue, Sep 17, 2013 at 11:31 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-09-17 09:45:28 -0400, Peter Eisentraut wrote:
>> On 9/15/13 11:30 AM, Andres Freund wrote:
>> > On 2013-09-15 11:20:20 -0400, Peter Eisentraut wrote:
>> >> On Sat, 2013-09-14 at 22:49 +0200, Andres Freund wrote:
>> >>> Attached you can find the newest version of the logical changeset
>> >>> generation patchset.
>> >>
>> >> You probably have bigger things to worry about, but please check the
>> >> results of cpluspluscheck, because some of the header files don't
>> >> include header files they depend on.
>> >
>> > Hm. I tried to get that right, but it's been a while since I last
>> > checked. I don't regularly use cpluspluscheck because it doesn't work in
>> > VPATH builds... We really need to fix that.
>> >
>> > I'll push a fix for that to the git tree, don't think that's worth a
>> > resend in itself.
>>
>> This patch set now fails to apply because of the commit "Rename various
>> "freeze multixact" variables".
>
> And I am even partially guilty for that patch...
>
> Rebased patches attached.

When I applied all the patches and do the compile, I got the following error:

gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith
-Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
-fwrapv -g -I. -I../../../../src/include -D_GNU_SOURCE   -c -o
snapbuild.o snapbuild.c
snapbuild.c:187: error: redefinition of typedef 'SnapBuild'
../../../../src/include/replication/snapbuild.h:45: note: previous
declaration of 'SnapBuild' was here
make[4]: *** [snapbuild.o] Error 1


When I applied only
0001-wal_decoding-Allow-walsender-s-to-connect-to-a-speci.patch,
compiled the source, and set up the asynchronous replication, I got
the segmentation
fault.
   LOG:  server process (PID 12777) was terminated by signal 11:
Segmentation fault

Regards,

-- 
Fujii Masao

Re: logical changeset generation v6

From

Andres Freund

Date:

19 September 2013, 11:24:57

On 2013-09-19 14:08:36 +0900, Fujii Masao wrote:
> When I applied all the patches and do the compile, I got the following error:
>
> gcc -O0 -Wall -Wmissing-prototypes -Wpointer-arith
> -Wdeclaration-after-statement -Wendif-labels
> -Wmissing-format-attribute -Wformat-security -fno-strict-aliasing
> -fwrapv -g -I. -I../../../../src/include -D_GNU_SOURCE   -c -o
> snapbuild.o snapbuild.c
> snapbuild.c:187: error: redefinition of typedef 'SnapBuild'
> ../../../../src/include/replication/snapbuild.h:45: note: previous
> declaration of 'SnapBuild' was here
> make[4]: *** [snapbuild.o] Error 1

Hm. Somebody had reported that previously and I tried to fix it but
obviously I failed. Unfortunately I don't see that warning in any of the
gcc versions I have tried locally.

Hopefully fixed.

> When I applied only
> 0001-wal_decoding-Allow-walsender-s-to-connect-to-a-speci.patch,
> compiled the source, and set up the asynchronous replication, I got
> the segmentation
> fault.

Fixed, I mismerged something, sorry for that.

Greetings,

Andres Freund

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

On 09/25/2013 11:08 AM, Andres Freund wrote:
> On 2013-09-25 11:01:44 -0400, Steve Singer wrote:
>> On 09/17/2013 10:31 AM, Andres Freund wrote:
>>> This patch set now fails to apply because of the commit "Rename various
>>> "freeze multixact" variables".
>>> And I am even partially guilty for that patch...
>>>
>>> Rebased patches attached.
>> While testing the logical replication changes against my WIP logical slony I
>> am sometimes getting error messages from the WAL sender of the form:
>> unexpected duplicate for tablespace  X relfilenode  X
> Any chance you could provide a setup to reproduce the error?
>

The steps to build a setup that should reproduce this error are:

1.  I had apply the attached patch on top of your logical replication
branch so my pg_decode_init  would now if it was being called as part of
a INIT_REPLICATION or START_REPLICATION.
Unless I have misunderstood something you probably will want to merge
this fix in

2.  Get my WIP for adding logical support to slony from:
git@github.com:ssinger/slony1-engine.git branch logical_repl
(4af1917f8418a)
(My code changes to slony are more prototype level code quality than
production code quality)

3.
cd slony1-engine
./configure --with-pgconfigdir=/usr/local/pg94wal/bin     (or whatever)
make
make install

4.   Grab the clustertest framework JAR from
https://github.com/clustertest/clustertest-framework and build up a
clustertest jar file

5.  Create a file
slony1-engine/clustertest/conf/java.conf
that contains the path to the above JAR file  as a shell variable
assignment: ie
CLUSTERTESTJAR=/home/ssinger/src/clustertest/clustertest_git/build/jar/clustertest-coordinator.jar

6.
cp clustertest/conf/disorder.properties.sample
clustertest/conf/disorder.properties

edit disorder.properites to have the proper values for your
environment.  All 6 databases can point at the same postgres instance,
this test will only actually use 2 of them(so far).

7. Run the test
cd clustertest
./run_all_disorder_tests.sh

This involves having the slon connect to the walsender on the database
test1 and replicate the data into test2 (which is a different database
on the same postmaster)

If this setup seems like too much effort I can request one of the
commitfest VM's from Josh and get everything setup there for you.

Steve

>> Any ideas?
> I'll look into it. Could you provide any context to what youre doing
> that's being decoded?
>
> Greetings,
>
> Andres Freund
>

Attachment

0001-set-the-init-parameter-to-true-when-performing-an-IN.patch

Re: logical changeset generation v6

From

Steve Singer

Date:

26 September 2013, 21:48:10

On 09/25/2013 01:20 PM, Steve Singer wrote:
> On 09/25/2013 11:08 AM, Andres Freund wrote:
>> On 2013-09-25 11:01:44 -0400, Steve Singer wrote:
>>> On 09/17/2013 10:31 AM, Andres Freund wrote:
>>>> This patch set now fails to apply because of the commit "Rename 
>>>> various
>>>> "freeze multixact" variables".
>>>> And I am even partially guilty for that patch...
>>>>
>>>> Rebased patches attached.
>>> While testing the logical replication changes against my WIP logical 
>>> slony I
>>> am sometimes getting error messages from the WAL sender of the form:
>>> unexpected duplicate for tablespace  X relfilenode  X
>> Any chance you could provide a setup to reproduce the error?
>>
>
> The steps to build a setup that should reproduce this error are:
>
> 1.  I had apply the attached patch on top of your logical replication 
> branch so my pg_decode_init  would now if it was being called as part 
> of a INIT_REPLICATION or START_REPLICATION.
> Unless I have misunderstood something you probably will want to merge 
> this fix in
>
> 2.  Get my WIP for adding logical support to slony from: 
> git@github.com:ssinger/slony1-engine.git branch logical_repl 
> (4af1917f8418a)
> (My code changes to slony are more prototype level code quality than 
> production code quality)
>
> 3.
> cd slony1-engine
> ./configure --with-pgconfigdir=/usr/local/pg94wal/bin     (or whatever)
> make
> make install
>
> 4.   Grab the clustertest framework JAR from 
> https://github.com/clustertest/clustertest-framework and build up a 
> clustertest jar file
>
> 5.  Create a file
> slony1-engine/clustertest/conf/java.conf
> that contains the path to the above JAR file  as a shell variable 
> assignment: ie
> CLUSTERTESTJAR=/home/ssinger/src/clustertest/clustertest_git/build/jar/clustertest-coordinator.jar 
>
>
> 6.
> cp clustertest/conf/disorder.properties.sample 
> clustertest/conf/disorder.properties
>
>
> edit disorder.properites to have the proper values for your 
> environment.  All 6 databases can point at the same postgres instance, 
> this test will only actually use 2 of them(so far).
>
> 7. Run the test
> cd clustertest
> ./run_all_disorder_tests.sh
>
> This involves having the slon connect to the walsender on the database 
> test1 and replicate the data into test2 (which is a different database 
> on the same postmaster)
>
> If this setup seems like too much effort I can request one of the 
> commitfest VM's from Josh and get everything setup there for you.
>
> Steve
>
>>> Any ideas?
>> I'll look into it. Could you provide any context to what youre doing
>> that's being decoded?
>>


I've determined that when in this test the walsender seems to be hitting 
this when it is decode the transactions that are behind the slonik 
commands to add tables to replication (set add table, set add 
sequence).  This is before the SUBSCRIBE SET is submitted.

I've also noticed something else that is strange (but might be 
unrelated).  If I stop my slon process and restart it I get messages like:

WARNING:  Starting logical replication from 0/a9321360
ERROR:  cannot stream from 0/A9321360, minimum is 0/A9320B00

Where 0/A9321360 was sent in the last packet my slon received from the 
walsender before the restart.

If force it to restart replication from 0/A9320B00 I see datarows that I 
appear to have already seen before the restart.
I think this is happening when I process the data for 0/A9320B00 but 
don't get the feedback message my slon was killed. Is this expected?



>> Greetings,
>>
>> Andres Freund
>>
>
>
>

Re: logical changeset generation v6.1

From

Andres Freund

Date:

27 September 2013, 18:14:52

Hi,

Attached you can find an updated version of the series taking in some of
the review comments (the others are queued, not ignored), including:
* split of things from the big "Introduce wal decoding via ..." patch
* fix the bug Steve notice where CreateLogicalDecodingContext was passed
  the wrong is_init = false where it should have been true
* A number of smaller bugs I noticed while reviewing
* Renaming of some variables, including guaranteedlyLogged ;)
* Comment improvements in decode.c
* rename pg_receivellog to pg_recvlogical

I'll work more on the other points in the next days, so far they are
clear of other big stuff.


0001 wal_decoding: Allow walsender's to connect to a specific database
- as before

0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
- as before

0003 wal_decoding: Add information about a tables primary key to struct RelationData
- as before

0004 wal_decoding: Add wal_level = logical and log data required for logical decoding
- splitof patch that contains the wal format changes including the
  addition of a new wal_level option

0005 wal_decoding: Add option to treat additional tables as catalog tables
- Option to treat user defined table as a catalog table which means it
  can be accessed during logical decoding from an output plugin

0006 wal_decoding: Introduce wal decoding via catalog timetravel
- The guts of changeset extraction, without a user interface

0007 wal_decoding: logical changeset extraction walsender interface
- splitof patch containing the walsender changes, which allow to receive
  the changeset data in a streaming fashion, supporting sync rep and
  such fancy things

0008 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding
- splitof optimization which reduces the pain 06 introduces by pegging
  the xmin horizon to the smallest of the logical decoding slots. Now
  it's pegged differently for data tables than from catalog tables

0009 wal_decoding: test_decoding: Add a simple decoding module in contrib
- Example output plugin which is also used in tests

0010 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes
- renamed client for the walsender interface

0011 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
- SQL SRF to get data from a decoding slot, also used as a vehicle for
  tests

0012 wal_decoding: design document v2.4 and snapshot building design doc v0.5

Greetings,

Andres Freund

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

Attachment

Re: logical changeset generation v6.1

From

Thom Brown

Date:

27 September 2013, 18:36:34

On 27 September 2013 16:14, Andres Freund <andres@2ndquadrant.com> wrote:
> Hi,
>
> Attached you can find an updated version of the series taking in some of
> the review comments (the others are queued, not ignored), including:
> * split of things from the big "Introduce wal decoding via ..." patch
> * fix the bug Steve notice where CreateLogicalDecodingContext was passed
>   the wrong is_init = false where it should have been true
> * A number of smaller bugs I noticed while reviewing
> * Renaming of some variables, including guaranteedlyLogged ;)
> * Comment improvements in decode.c
> * rename pg_receivellog to pg_recvlogical
>
> I'll work more on the other points in the next days, so far they are
> clear of other big stuff.
>
>
> 0001 wal_decoding: Allow walsender's to connect to a specific database
> - as before
>
> 0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
> - as before
>
> 0003 wal_decoding: Add information about a tables primary key to struct RelationData
> - as before
>
> 0004 wal_decoding: Add wal_level = logical and log data required for logical decoding
> - splitof patch that contains the wal format changes including the
>   addition of a new wal_level option
>
> 0005 wal_decoding: Add option to treat additional tables as catalog tables
> - Option to treat user defined table as a catalog table which means it
>   can be accessed during logical decoding from an output plugin
>
> 0006 wal_decoding: Introduce wal decoding via catalog timetravel
> - The guts of changeset extraction, without a user interface
>
> 0007 wal_decoding: logical changeset extraction walsender interface
> - splitof patch containing the walsender changes, which allow to receive
>   the changeset data in a streaming fashion, supporting sync rep and
>   such fancy things
>
> 0008 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding
> - splitof optimization which reduces the pain 06 introduces by pegging
>   the xmin horizon to the smallest of the logical decoding slots. Now
>   it's pegged differently for data tables than from catalog tables
>
> 0009 wal_decoding: test_decoding: Add a simple decoding module in contrib
> - Example output plugin which is also used in tests
>
> 0010 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes
> - renamed client for the walsender interface
>
> 0011 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
> - SQL SRF to get data from a decoding slot, also used as a vehicle for
>   tests
>
> 0012 wal_decoding: design document v2.4 and snapshot building design doc v0.5

I'm encountering a make error:

install  pg_basebackup '/home/thom/Development/psql/bin/pg_basebackup'
install  pg_receivexlog '/home/thom/Development/psql/bin/pg_receivexlog'
install  pg_recvlogical(X) '/home/thom/Development/psql/bin/pg_receivellog'
/bin/dash: 1: Syntax error: "(" unexpected
make[3]: *** [install] Error 2
make[3]: Leaving directory
`/home/thom/Development/postgresql/src/bin/pg_basebackup'
make[2]: *** [install-pg_basebackup-recurse] Error 2

Thom

Re: logical changeset generation v6.1

From

Andres Freund

Date:

27 September 2013, 18:44:13

On 2013-09-27 16:35:53 +0100, Thom Brown wrote:
> On 27 September 2013 16:14, Andres Freund <andres@2ndquadrant.com> wrote:
> > Hi,
> >
> > Attached you can find an updated version of the series taking in some of
> > the review comments (the others are queued, not ignored), including:
> > * split of things from the big "Introduce wal decoding via ..." patch
> > * fix the bug Steve notice where CreateLogicalDecodingContext was passed
> >   the wrong is_init = false where it should have been true
> > * A number of smaller bugs I noticed while reviewing
> > * Renaming of some variables, including guaranteedlyLogged ;)
> > * Comment improvements in decode.c
> > * rename pg_receivellog to pg_recvlogical
> >
> > I'll work more on the other points in the next days, so far they are
> > clear of other big stuff.
> >
> >
> > 0001 wal_decoding: Allow walsender's to connect to a specific database
> > - as before
> >
> > 0002 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
> > - as before
> >
> > 0003 wal_decoding: Add information about a tables primary key to struct RelationData
> > - as before
> >
> > 0004 wal_decoding: Add wal_level = logical and log data required for logical decoding
> > - splitof patch that contains the wal format changes including the
> >   addition of a new wal_level option
> >
> > 0005 wal_decoding: Add option to treat additional tables as catalog tables
> > - Option to treat user defined table as a catalog table which means it
> >   can be accessed during logical decoding from an output plugin
> >
> > 0006 wal_decoding: Introduce wal decoding via catalog timetravel
> > - The guts of changeset extraction, without a user interface
> >
> > 0007 wal_decoding: logical changeset extraction walsender interface
> > - splitof patch containing the walsender changes, which allow to receive
> >   the changeset data in a streaming fashion, supporting sync rep and
> >   such fancy things
> >
> > 0008 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding
> > - splitof optimization which reduces the pain 06 introduces by pegging
> >   the xmin horizon to the smallest of the logical decoding slots. Now
> >   it's pegged differently for data tables than from catalog tables
> >
> > 0009 wal_decoding: test_decoding: Add a simple decoding module in contrib
> > - Example output plugin which is also used in tests
> >
> > 0010 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes
> > - renamed client for the walsender interface
> >
> > 0011 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
> > - SQL SRF to get data from a decoding slot, also used as a vehicle for
> >   tests
> >
> > 0012 wal_decoding: design document v2.4 and snapshot building design doc v0.5
>
> I'm encountering a make error:

Gah. Lastminute changes. Always the same... Updated patch attached.

Greetings,

Andres Freund

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

Attachment

0010-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz

Re: logical changeset generation v6

From

Steve Singer

Date:

28 September 2013, 00:07:08

On 09/26/2013 02:47 PM, Steve Singer wrote:
>
>
> I've determined that when in this test the walsender seems to be 
> hitting this when it is decode the transactions that are behind the 
> slonik commands to add tables to replication (set add table, set add 
> sequence).  This is before the SUBSCRIBE SET is submitted.
>
> I've also noticed something else that is strange (but might be 
> unrelated).  If I stop my slon process and restart it I get messages 
> like:
>
> WARNING:  Starting logical replication from 0/a9321360
> ERROR:  cannot stream from 0/A9321360, minimum is 0/A9320B00
>
> Where 0/A9321360 was sent in the last packet my slon received from the 
> walsender before the restart.
>
> If force it to restart replication from 0/A9320B00 I see datarows that 
> I appear to have already seen before the restart.
> I think this is happening when I process the data for 0/A9320B00 but 
> don't get the feedback message my slon was killed. Is this expected?
>
>

I've further narrowed this down to something (or the combination of) 
what the  _disorder_replica.altertableaddTriggers(1);
stored function does.  (or @SLONYNAMESPACE@.altertableaddTriggers(int);

Which is essentially
* Get an exclusive lock on sl_config_lock
* Get an exclusive lock on the user table in question
* create a trigger (the deny access trigger)
* create a truncate trigger
* create a deny truncate trigger

I am not yet able to replicate the error by issuing the same SQL 
commands from psql, but I must be missing something.

I can replicate this when just using the test_decoding plugin.








>
>>> Greetings,
>>>
>>> Andres Freund
>>>
>>
>>
>>
>
>
>

Re: logical changeset generation v6

From

Andres Freund

Date:

28 September 2013, 00:18:32

Hi Steve,

On 2013-09-27 17:06:59 -0400, Steve Singer wrote:
> >I've determined that when in this test the walsender seems to be hitting
> >this when it is decode the transactions that are behind the slonik
> >commands to add tables to replication (set add table, set add sequence).
> >This is before the SUBSCRIBE SET is submitted.
> >
> >I've also noticed something else that is strange (but might be unrelated).
> >If I stop my slon process and restart it I get messages like:
> >
> >WARNING:  Starting logical replication from 0/a9321360
> >ERROR:  cannot stream from 0/A9321360, minimum is 0/A9320B00
> >
> >Where 0/A9321360 was sent in the last packet my slon received from the
> >walsender before the restart.

Uh, that looks like I fumbled some comparison. Let me check.

> I've further narrowed this down to something (or the combination of) what
> the  _disorder_replica.altertableaddTriggers(1);
> stored function does.  (or @SLONYNAMESPACE@.altertableaddTriggers(int);
> 
> Which is essentially
> * Get an exclusive lock on sl_config_lock
> * Get an exclusive lock on the user table in question
> * create a trigger (the deny access trigger)
> * create a truncate trigger
> * create a deny truncate trigger
> 
> I am not yet able to replicate the error by issuing the same SQL commands
> from psql, but I must be missing something.
> 
> I can replicate this when just using the test_decoding plugin.

Thanks. That should get me started with debugging. Unless it's possibly
fixed in the latest version, one bug fixed there might cause something
like this if the moon stands exactly right?

Greetings,

Andres Freund

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

Re: logical changeset generation v6.1

From

Steve Singer

Date:

28 September 2013, 19:55:53

On 09/27/2013 11:44 AM, Andres Freund wrote:
> I'm encountering a make error:
> Gah. Lastminute changes. Always the same... Updated patch attached.
>
> Greetings,
>
> Andres Freund
>
>

I'm still encountering an error in the make.

make clean
.
.make[3]: Entering directory 
`/usr/local/src/postgresql/src/bin/pg_basebackup'
rm -f pg_basebackup pg_receivexlog pg_recvlogical(X) \        pg_basebackup.o pg_receivexlog.o pg_recvlogical.o \
receivelog.o streamutil.o

/bin/sh: 1: Syntax error: "(" unexpected
make[3]: *** [clean] Error 2

I had to add a quotes in to the clean commands to make it work

>

Re: logical changeset generation v6

From

Steve Singer

Date:

28 September 2013, 20:25:16

On 09/27/2013 05:18 PM, Andres Freund wrote:
> Hi Steve,
>
> On 2013-09-27 17:06:59 -0400, Steve Singer wrote:
>>> I've determined that when in this test the walsender seems to be hitting
>>> this when it is decode the transactions that are behind the slonik
>>> commands to add tables to replication (set add table, set add sequence).
>>> This is before the SUBSCRIBE SET is submitted.
>>>
>>> I've also noticed something else that is strange (but might be unrelated).
>>> If I stop my slon process and restart it I get messages like:
>>>
>>> WARNING:  Starting logical replication from 0/a9321360
>>> ERROR:  cannot stream from 0/A9321360, minimum is 0/A9320B00
>>>
>>> Where 0/A9321360 was sent in the last packet my slon received from the
>>> walsender before the restart.
> Uh, that looks like I fumbled some comparison. Let me check.
>
>> I've further narrowed this down to something (or the combination of) what
>> the  _disorder_replica.altertableaddTriggers(1);
>> stored function does.  (or @SLONYNAMESPACE@.altertableaddTriggers(int);
>>
>> Which is essentially
>> * Get an exclusive lock on sl_config_lock
>> * Get an exclusive lock on the user table in question
>> * create a trigger (the deny access trigger)
>> * create a truncate trigger
>> * create a deny truncate trigger
>>
>> I am not yet able to replicate the error by issuing the same SQL commands
>> from psql, but I must be missing something.
>>
>> I can replicate this when just using the test_decoding plugin.
> Thanks. That should get me started with debugging. Unless it's possibly
> fixed in the latest version, one bug fixed there might cause something
> like this if the moon stands exactly right?

The latest version has NOT fixed the problem.

Also, I was a bit inaccurate in my previous descriptions. To clarify:

1.   I sometimes am getting that 'unexpected duplicate' error
2.   The 'set add table ' which triggers those functions that create and 
configure triggers is actually causing the walsender to hit the 
following assertion
2  0x0000000000773d47 in ExceptionalCondition (    conditionName=conditionName@entry=0x8cf400 "!(ent->cmin == 
change->tuplecid.cmin)", errorType=errorType@entry=0x7ab830 
"FailedAssertion",    fileName=fileName@entry=0x8cecc3 "reorderbuffer.c",    lineNumber=lineNumber@entry=1162) at
assert.c:54
#3  0x0000000000665480 in ReorderBufferBuildTupleCidHash (txn=0x1b6e610,    rb=<optimized out>) at
reorderbuffer.c:1162
#4  ReorderBufferCommit (rb=0x1b6e4f8, xid=<optimized out>,    commit_lsn=3461001952, end_lsn=<optimized out>) at
reorderbuffer.c:1285
#5  0x000000000065f0f7 in DecodeCommit (xid=<optimized out>,    nsubxacts=<optimized out>, sub_xids=<optimized out>,
ninval_msgs=16,   msgs=0x1b637c0, buf=0x7fff54d01530, buf=0x7fff54d01530, ctx=0x1adb928,    ctx=0x1adb928) at
decode.c:477

I had added an assert(false) to the code where the 'unknown duplicate' 
error was logged to make spotting this easier but yesterday I didn't 
double check that I was hitting the assertion I added versus this other 
one.   I can't yet say if this is two unrelated issues or if I'd get to 
the 'unknown duplicate' message immediately after.

> Greetings,
>
> Andres Freund
>

Re: logical changeset generation v6.1

From

Alvaro Herrera

Date:

29 September 2013, 02:56:30

Steve Singer wrote:

> I'm still encountering an error in the make.
> 
> make clean
> .
> .make[3]: Entering directory
> `/usr/local/src/postgresql/src/bin/pg_basebackup'
> rm -f pg_basebackup pg_receivexlog pg_recvlogical(X) \
>         pg_basebackup.o pg_receivexlog.o pg_recvlogical.o \
>         receivelog.o streamutil.o
> /bin/sh: 1: Syntax error: "(" unexpected
> make[3]: *** [clean] Error 2
> 
> I had to add a quotes in to the clean commands to make it work

The proper fix is to add a $ to the pg_recvlogical(X) in "clean" -- should be $(X)

There's another bug in the Makefile: the install target is installing
recvlogical$(X) as receivellog$(X).

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Re: logical changeset generation v6.1

From

Kevin Grittner

Date:

01 October 2013, 01:00:47

Andres Freund <andres@2ndquadrant.com> wrote:

> Attached you can find an updated version of the series taking in some of
> the review comments

I don't know whether this is related to the previously-reported
build problems, but when I apply each patch in turn, with make -j4
world && make check-world for each step, I die during compile of
0004.

make[4]: Entering directory `/home/kgrittn/pg/master/src/backend/access/transam'
gcc -O2 -Wall -Wmissing-prototypes -Wpointer-arith -Wdeclaration-after-statement -Wendif-labels
-Wmissing-format-attribute-Wformat-security -fno-strict-aliasing -fwrapv -fexcess-precision=standard -g
-I../../../../src/include-D_GNU_SOURCE -I/usr/include/libxml2   -c -o xlog.o xlog.c -MMD -MP -MF .deps/xlog.Po 
xlog.c:44:33: fatal error: replication/logical.h: No such file or directory
compilation terminated.
make[4]: *** [xlog.o] Error 1

I tried maintainer-clean and a new ./configure to see if that would
get me past it; no joy.  I haven't dug further, but if this is not
a known issue I can poke around.  If it is known -- how do I get
past it?

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: logical changeset generation v6.2

From

Andres Freund

Date:

01 October 2013, 01:44:42

Hi,

The series from friday was a bit too buggy - obviously I was too
tired. So here's a new one:

* fix pg_recvlogical makefile (Thanks Steve)
* fix two commits not compiling properly without later changes (Thanks Kevin)
* keep track of commit timestamps
* fix bugs with option passing in test_logical_decoding
* actually parse option values in test_decoding instead of just using the
  option name
* don't use anonymous structs in unions. That's compiler specific (msvc
  and gcc) before C11 on which we can't rely. That unfortunately will
  break output plugins because ReorderBufferChange need to qualify
  old/new tuples now
* improve error handling/cleanup in test_logical_decoding
* some minor cleanups

Patches attached, git tree updated.

Greetings,

Andres Freund

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

Hi,

On 2013-10-01 10:07:19 -0400, Robert Haas wrote:
> - It seems that HeapSatisfiesHOTandKeyUpdate is now
> HeapSatisfiesHOTandKeyandCandidateKeyUpdate.  Considering I think this
> was merely HeapSatisfiesHOTUpdate a year ago, it's hard not to be
> afraid that something unscalable is happening to this function.  On a
> related node, any overhead added here costs broadly; I'm not sure if
> there's enough to worry about.

I haven't changed anything here - ISTM so far nobody had a better
suggestion.

> - RelationIsDoingTimetravel is still a crappy name.  How about
> RelationRequiredForLogicalDecoding?  And maybe the reloption
> treat_as_catalog_table can become required_for_logical_decoding.

Hm. I don't really like the name, required seems to imply that it's
necessary to turn this on to get data replicated in that relation. How
about "accessible_during_logical_decoding" or "user_catalog_table"? The
latter would allow us to use it to add checks for user relations used in
indexes which need a treatment similar to enums.

> All in all, it seems to me that we shouldn't try to punt.  Maybe we
> should have something that works like ALTER TABLE name CLUSTER ON
> index_name to configure which index should be used for logical
> replication.  Possibly this same syntax could be used as ALTER
> MATERIALIZED VIEW to set the candidate key for that case.

How about using the current logic by default but allow to tune it
additionally with an option like that?

So, attached is the new version:
Changes:
* Fix issues you noticed except the above
* Handle multixacts on system tables
* Logical slots now are checksummed and contain a version and length
* Improve logic for increasing the "restart lsn", the point where we
  start to read WAL to decode from next time round
* Wait for xids in snapbuild, during the initial build
* s/RelationIsDoingTimetravel/RelationRequiredForLogicalDecoding/
* test_logical_decoding: confirm reception of changes at the end
* prohibit rewriting schema changes for treat_as_catalog_table relations
* add tests for dropping/adding primary/candidate keys
* PROCESS_INTERRUPTS whene reading wal for SQL SRF
* cleanup old serialized snapshots at check/restart points
* Add more isolationtester changes

Todo:
* rename treat_as_catalog_table, after agreeing on the new name
* rename remaining timetravel function names
* restrict SuspendDecodingSnapshots usage to RelationInitPhysicalAddr,
  that ought to be enough.
* add InLogicalDecoding() function.
* throw away older data when reading xl_running_xacts records, to deal
  with immediate shutdowns/crashes

Greetings,

Andres Freund

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

Hi,

Attached you can find version 6.4 of the patchset:

* reordered so the patches Robert wants to apply first are first
* renamed treat_as_catalog_table to user_catalog_table
* renamed RelationRequiredForLogicalDecoding to RelationIsAccessibleInLogicalDecoding
* moved two hunks to better fitting patches

I am working on the longer TODOs from the last version now, but they
don't affect the first patches.

Greetings,

Andres Freund

PS: git rebase -i -x /path/to/testscript is cool

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

On 2013-10-14 15:51:14 +0200, Andres Freund wrote:
> > > It'd probably not hurt to redo those benchmarks to make sure...
> >
> > Yes, I think it would be good to characterize it more precisely than
> > "a bit", so people know what to expect.
>
> A "bit" was below the 3% range for loops of adding columns.
>
> So, any tests you'd like to see?
> * loop around CREATE TABLE/DROP TABLE
> * loop around ALTER TABLE ... ADD COLUMN
> * loop around CREATE FUNCTION/DROP FUNCTION

So, see the attatched benchmark skript. I've always done using a disk
bound and a memory bound (using eatmydata, preventing fsyncs) run.

* unpatched run, wal_level = hot_standby, eatmydata
* unpatched run, wal_level = hot_standby

* patched run, wal_level = hot_standby, eatmydata
* patched run, wal_level = hot_standby

* patched run, wal_level = logical, eatmydata
* patched run, wal_level = logical

Based on those results, there's no difference above noise for
wal_level=hot_standby, with or without the patch. With wal_level=logical
there's a measurable increase in wal traffic (~12-17%), but no
performance decrease above noise.

>From my POV that's ok, those are really crazy catalog workloads.

Greetings,

Andres Freund

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

On 2013-10-21 16:15:58 +0200, Andres Freund wrote:
> On 2013-10-21 09:32:12 -0400, Robert Haas wrote:
> > On Fri, Oct 18, 2013 at 2:26 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > > I know of the following solutions:
> > > 1) Don't allow VACUUM FULL on catalog tables if wal_level = logical.
> > > 2) Make VACUUM FULL prevent DDL and then wait till all changestreams
> > >    have decoded up to the current point.
> > > 3) don't delete the old relfilenode for VACUUM/CLUSTERs of system tables
> > >    if there are life decoding slots around, instead delegate that
> > >    responsibility to the slot management.
> > > 4) Store both (cmin, cmax) for catalog tuples.
> > >
> > > I bascially think only 1) and 4) are realistic. And 1) sucks.
> > >
> > > I've developed a prototype for 4) and except currently being incredibly
> > > ugly, it seems to be the most promising approach by far. My trick to
> > > store both cmin and cmax is to store cmax in t_hoff managed space when
> > > wal_level = logical.
> >
> > In my opinion, (4) is too ugly to consider.  I think that if we start
> > playing games like this, we're opening up the doors to lots of subtle
> > bugs and future architectural pain that will be with us for many, many
> > years to come.  I believe we will bitterly regret any foray into this
> > area.
>
> Hm. After looking at the required code - which you obviously cannot have
> yet - it's not actually too bad. Will post a patch implementing it later.
>
> I don't really buy the architectural argument since originally cmin/cmax
> *were* both stored. It's not something we're just inventing now. We just
> optimized that away but now have discovered that's not always a good
> idea and thus don't always use the optimization.
>
> The actual decoding code shrinks by about 200 lines using this logic
> which is a hint that it's not a bad idea.

So, here's a preliminary patch to see how this would look. It'd be great
of you comment if you still think it's a completel no-go.

If it were for real, it'd need to be split and some minor things would
need to get adjusted, but I think it's easier to review it seing both
sides at once.

Greetings,

Andres Freund

PS: The patch is ontop of a new git push, but for review that shouldn't
matter.

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

Attachment

0001-wal_decoding-Rewrite-CommandId-handling.patch

Re: logical changeset generation v6.4

From

Andres Freund

Date:

21 October 2013, 21:16:39

On 2013-10-18 20:50:58 +0200, Andres Freund wrote:
> How about modifying the selection to go from:
> * all rows if ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL;
> * index chosen by ALTER TABLE ... REPLICA IDENTITY USING indexname
> * [later, maybe] ALTER TABLE ... REPLICA IDENTITY (cola, colb)

Current draft is:
ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL|DEFAULT
ALTER TABLE ... REPLICA IDENTITY USING INDEX ...;

which leaves the door open for

ALTER TABLE ... REPLICA IDENTITY USING '(' column_name_list ')';

Does anybody have a strong feeling about requiring support for CREATE
TABLE for this?

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

21 October 2013, 21:22:23

On Mon, Oct 21, 2013 at 1:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > In my opinion, (4) is too ugly to consider.  I think that if we start
>> > playing games like this, we're opening up the doors to lots of subtle
>> > bugs and future architectural pain that will be with us for many, many
>> > years to come.  I believe we will bitterly regret any foray into this
>> > area.
>>
>> Hm. After looking at the required code - which you obviously cannot have
>> yet - it's not actually too bad. Will post a patch implementing it later.
>>
>> I don't really buy the architectural argument since originally cmin/cmax
>> *were* both stored. It's not something we're just inventing now. We just
>> optimized that away but now have discovered that's not always a good
>> idea and thus don't always use the optimization.
>>
>> The actual decoding code shrinks by about 200 lines using this logic
>> which is a hint that it's not a bad idea.
>
> So, here's a preliminary patch to see how this would look. It'd be great
> of you comment if you still think it's a completel no-go.
>
> If it were for real, it'd need to be split and some minor things would
> need to get adjusted, but I think it's easier to review it seing both
> sides at once.

I think it's a complete no-go.  Consider, e.g., the comment for
MaxTupleAttributeNumber, which you've blithely falsified.  Even if you
update the comment and the value, I'm not inspired by the idea of
subtracting 32 from that number; even if it weren't already too small,
it would break pg_upgrade for any users who are on the edge.  Things
aren't looking too good for anything that uses HeapTupleFields,
either; consider rewrite_heap_tuple().

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

21 October 2013, 21:27:44

On 2013-10-21 14:22:17 -0400, Robert Haas wrote:
> On Mon, Oct 21, 2013 at 1:52 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> > In my opinion, (4) is too ugly to consider.  I think that if we start
> >> > playing games like this, we're opening up the doors to lots of subtle
> >> > bugs and future architectural pain that will be with us for many, many
> >> > years to come.  I believe we will bitterly regret any foray into this
> >> > area.
> >>
> >> Hm. After looking at the required code - which you obviously cannot have
> >> yet - it's not actually too bad. Will post a patch implementing it later.
> >>
> >> I don't really buy the architectural argument since originally cmin/cmax
> >> *were* both stored. It's not something we're just inventing now. We just
> >> optimized that away but now have discovered that's not always a good
> >> idea and thus don't always use the optimization.
> >>
> >> The actual decoding code shrinks by about 200 lines using this logic
> >> which is a hint that it's not a bad idea.
> >
> > So, here's a preliminary patch to see how this would look. It'd be great
> > of you comment if you still think it's a completel no-go.
> >
> > If it were for real, it'd need to be split and some minor things would
> > need to get adjusted, but I think it's easier to review it seing both
> > sides at once.
> 
> I think it's a complete no-go.  Consider, e.g., the comment for
> MaxTupleAttributeNumber, which you've blithely falsified.  Even if you
> update the comment and the value, I'm not inspired by the idea of
> subtracting 32 from that number; even if it weren't already too small,
> it would break pg_upgrade for any users who are on the edge.

Well, we only need to support it for (user_)catalog tables. So
pg_upgrade isn't a problem. And I don't really see a problem restricting
the number of columns for such tables.

> Things
> aren't looking too good for anything that uses HeapTupleFields,
> either; consider rewrite_heap_tuple().

Well, that currently works, by copying cmax. Since rewriting triggered
the change, I am pretty sure I've actually tested & hit that path...

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

21 October 2013, 21:51:01

On Mon, Oct 21, 2013 at 2:27 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> I think it's a complete no-go.  Consider, e.g., the comment for
>> MaxTupleAttributeNumber, which you've blithely falsified.  Even if you
>> update the comment and the value, I'm not inspired by the idea of
>> subtracting 32 from that number; even if it weren't already too small,
>> it would break pg_upgrade for any users who are on the edge.
>
> Well, we only need to support it for (user_)catalog tables. So
> pg_upgrade isn't a problem. And I don't really see a problem restricting
> the number of columns for such tables.

Inch by inch, this patch set is trying to make catalog tables more and
more different from regular tables.  I think that's a direction we're
going to regret.  I can almost believe that no great harm will come to
us from giving the two different xmin horizons, as previously
discussed, though I'm still not 100% happy about that.  Can't both
have something be a catalog table AND replicate it?  Ick, but OK.

But changing the on disk format strikes me as crossing some sort of
bright line.  That means that you're going to have two different code
paths in a lot of important cases, one for catalog tables and one for
non-catalog tables, and the one that's only taken for catalog tables
will be rather lightly traveled.  And then you've got user catalog
tables, and the possibility that something that wasn't formerly a user
catalog table might become one, or visca versa.  Even if you can flush
out every bug that exists today, this is a recipe for future bugs.

>> Things
>> aren't looking too good for anything that uses HeapTupleFields,
>> either; consider rewrite_heap_tuple().
>
> Well, that currently works, by copying cmax. Since rewriting triggered
> the change, I am pretty sure I've actually tested & hit that path...

No offense, but that's a laughable statement.  If that path works,
it's mostly if not entirely by accident.  You've fundamentally changed
the heap tuple format, and that code doesn't know about it, even
though it's deeply in bed with assumptions about the old format.  I
think this is a pretty clear indication as to what's wrong with this
approach: a lot of stuff will not care, but the stuff that does care
will be hard to find, and future incremental modifications either to
that code or to the hidden data before the t_hoff pointer could break
stuff that formerly worked.  We rejected early drafts of sepgsql RLS
cold because they changed the tuple format, and I don't see why we
shouldn't do exactly the same thing here.

But just suppose for a minute that we'd accepted that proposal and
then took this one, too.  And let's suppose we also accept the next
proposal that, like that one and this one, jams something more into
the heap tuple header.  At that point you'll have potentially as many
as 8 different maximum-number-of-attributes values for tuples, though
maybe not quite that many in practice if not all of those features can
be used together.  The macros that are needed to extract the various
values from the heap tuple will be nightmarishly complex, and we'll
have eaten up all (or more than all) of our remaining bit-space in the
infomask.  Maybe all of that sounds all right to you, but to me it
sounds like a big mess.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

21 October 2013, 22:36:08

On 2013-10-21 14:50:54 -0400, Robert Haas wrote:
> On Mon, Oct 21, 2013 at 2:27 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> I think it's a complete no-go.  Consider, e.g., the comment for
> >> MaxTupleAttributeNumber, which you've blithely falsified.  Even if you
> >> update the comment and the value, I'm not inspired by the idea of
> >> subtracting 32 from that number; even if it weren't already too small,
> >> it would break pg_upgrade for any users who are on the edge.
> >
> > Well, we only need to support it for (user_)catalog tables. So
> > pg_upgrade isn't a problem. And I don't really see a problem restricting
> > the number of columns for such tables.
> 
> Inch by inch, this patch set is trying to make catalog tables more and
> more different from regular tables.  I think that's a direction we're
> going to regret.

I can understand that.

> Can't both have something be a catalog table AND replicate it?  Ick,
> but OK.

User catalog tables are replicated normally.

If we want we can replicate system catalog tables as well, ok, it's
about a days worth of work. I just don't see what the use case would
be and I am afraid it'd be used as a poor man's system table trigger.

> But changing the on disk format strikes me as crossing some sort of
> bright line.  That means that you're going to have two different code
> paths in a lot of important cases, one for catalog tables and one for
> non-catalog tables, and the one that's only taken for catalog tables
> will be rather lightly traveled.

But there really isn't that much difference in the code paths, is there?
If you look at it, minus the mechanical changes around bitmasks it
really mostly just a couple of added
HeapTupleHeaderSetCmin/HeapTupleHeaderSetCmax calls + wal logging the
current cid.

> And then you've got user catalog tables, and the possibility that
> something that wasn't formerly a user catalog table might become one,
> or visca versa.

That's not really a problem - we only need cmin/cmax for catalog tables
when decoding DML that happened in the same transaction as DDL. The wide
cids could easily be removed when we freeze tuples or such.
Even something like:
BEGIN;
CREATE TABLE catalog_t(...);
INSERT INTO catalog_t ...;
UPDATE catalog_t ...
ALTER TABLE catalog_t SET (user_catalog_table = true);
INSERT INTO decoded_table (..);
INSERT INTO decoded_table (..);
UPDATE catalog_t SET ...;
INSERT INTO decoded_table (..);
COMMIT;
will work correctly.

> Even if you can flush out every bug that exists today, this is a
> recipe for future bugs.

I don't really forsee many new codepaths that care about the visibility
bits in tuple headers themselves. When has the last one of those been
added/changed? I'd bet it was 9.0 with the vacuum full/cluster merge,
and three lines to be added are surely the smallest problem of that.

> >> Things
> >> aren't looking too good for anything that uses HeapTupleFields,
> >> either; consider rewrite_heap_tuple().
> >
> > Well, that currently works, by copying cmax. Since rewriting triggered
> > the change, I am pretty sure I've actually tested & hit that path...
> 
> No offense, but that's a laughable statement.  If that path works,
> it's mostly if not entirely by accident.  You've fundamentally changed
> the heap tuple format, and that code doesn't know about it, even
> though it's deeply in bed with assumptions about the old format.

Hm? rewrite_heap_tuple() grew code to handle that case, it's not an
accident that it works.
if (HeapTupleHeaderHasWideCid(old_tuple->t_data)           && HeapTupleHeaderHasWideCid(new_tuple->t_data)){
HeapTupleHeaderSetCmin(new_tuple->t_data,
HeapTupleHeaderGetCmin(old_tuple->t_data));   HeapTupleHeaderSetCmax(new_tuple->t_data,
    HeapTupleHeaderGetCmax(old_tuple->t_data), false);}

Note that code handling HeapTupleHeaders outside of heapam.c, tqual.c et
al. shouldn't need to care about the changes at all.
And all the code that needs to care *already* has special-cased code
around visibility. It's relatively easy to find by grepping for
HEAP_XACT_MASK. We've so far accepted that several places need to change
if we change the visibility rules. And I don't see how we easily could
get away from that.
Patches like e.g. lsn-ranges freezing will require *gobs* more
widespread changes. And have much higher chances of breaking stuff
imo. And it's still worthwile to do them.

> I
> think this is a pretty clear indication as to what's wrong with this
> approach: a lot of stuff will not care, but the stuff that does care
> will be hard to find, and future incremental modifications either to
> that code or to the hidden data before the t_hoff pointer could break
> stuff that formerly worked.  We rejected early drafts of sepgsql RLS
> cold because they changed the tuple format, and I don't see why we
> shouldn't do exactly the same thing here.

Ok, I have a hard time to argue against that. I either skipped or forgot
that discussion.
I quickly searched and looked at the patch at:
497D7055.9090806@ak.jp.nec.com From a quick look that would have grown
much more invasive - and lots more places would have had to care. For
the proposed patch it really is only
heap_(insert|multi_insert|update|delete|rewrite_tuple) that need to
care. We could remove the tupledesc changes if we dont care about some
added malloc/memcpy'ing during DDL, the code to handle tuples without
that space is already there.

> But just suppose for a minute that we'd accepted that proposal and
> then took this one, too.  And let's suppose we also accept the next
> proposal that, like that one and this one, jams something more into
> the heap tuple header.  At that point you'll have potentially as many
> as 8 different maximum-number-of-attributes values for tuples, though
> maybe not quite that many in practice if not all of those features can
> be used together.

Well, I can understand that argument. Unfortunately. But I still claim
that this is scaling back an optimization that we've previously applied
more widely.

Note also that we actually have quite some slop in
MaxTupleAttributeNumber and MaxHeapAttributeNumber (which is the one
that matters here). They e.g. haven't been adjusted when 8.3 decided
*not* to store both cmin and cmax anymore and they *already* had slop
before. So I am not convinced that we actually need to change our
current limits.

> The macros that are needed to extract the various
> values from the heap tuple will be nightmarishly complex, and we'll
> have eaten up all (or more than all) of our remaining bit-space in the
> infomask.

Why would the macros for extracting values from heap tuples need to
change? Those shouldn't be affected by what I proposed?

I think this approach would have lower maintenance overhead in
comparison to the previous solution of Handling CommandIds because it's
actually much simpler. But I am definitely ready to try other ideas.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

21 October 2013, 22:41:59

Hi,

On 2013-10-21 21:36:02 +0200, Andres Freund wrote:
> I think this approach would have lower maintenance overhead in
> comparison to the previous solution of Handling CommandIds because it's
> actually much simpler.

Btw, I think the new approach would allow for *easier* modifications
about future code caring about visibility. Since the physical location
doesn't matter anymore it'd be much more friendly towards things like
an online compacting VACUUM and such.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.4

From

Andres Freund

Date:

22 October 2013, 17:07:30

On 2013-10-21 20:16:29 +0200, Andres Freund wrote:
> On 2013-10-18 20:50:58 +0200, Andres Freund wrote:
> > How about modifying the selection to go from:
> > * all rows if ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL;
> > * index chosen by ALTER TABLE ... REPLICA IDENTITY USING indexname
> > * [later, maybe] ALTER TABLE ... REPLICA IDENTITY (cola, colb)
>
> Current draft is:
> ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL|DEFAULT
> ALTER TABLE ... REPLICA IDENTITY USING INDEX ...;
>
> which leaves the door open for
>
> ALTER TABLE ... REPLICA IDENTITY USING '(' column_name_list ')';
>
> Does anybody have a strong feeling about requiring support for CREATE
> TABLE for this?

Attached is a patch ontop of master implementing this syntax. It's not
wired up to the changeset extraction patch yet as I am not sure whether
others agree about the storage.

pg_class grew a 'relreplident' char, storing:
* 'd' default
* 'n' nothing
* 'f' full
* 'i' index with indisreplident set, or default if index has been
      dropped
pg_index grew a 'indisreplident' bool indicating it is set as the
replica identity for a replident = 'i' relation.

Both changes shouldn't change the width of the affected relations, they
should reuse existing padding.

Does somebody prefer a different storage?

pg_dump support, psql completion, regression tests and minimal docs
included.

I am not 100% clear what the best way to handle
ALTER TABLE some_table REPLICA IDENTITY USING INDEX someindex;
DROP INDEX someindex;
is. Currently that's supposed to have the same effect as having
relreplident = 'd'.

Greetings,

Andres Freund

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

Attachment

0001-Add-command-to-set-the-REPLICA-IDENTITY-for-a-table.patch

Re: logical changeset generation v6.2

From

Robert Haas

Date:

22 October 2013, 17:52:58

On Fri, Oct 18, 2013 at 2:26 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> So. As it turns out that solution isn't sufficient in the face of VACUUM
> FULL and mixed DML/DDL transaction that have not yet been decoded.
>
> To reiterate, as published it works like:
> For every modification of catalog tuple (insert, multi_insert, update,
> delete) that has influence over visibility issue a record that contains:
> * filenode
> * ctid
> * (cmin, cmax)
>
> When doing a visibility check on a catalog row during decoding of mixed
> DML/DDL transaction lookup (cmin, cmax) for that row since we don't
> store both for the tuple.
>
> That mostly works great.
>
> The problematic scenario is decoding a transaction that has done mixed
> DML/DDL *after* a VACUUM FULL/CLUSTER has been performed. The VACUUM
> FULL obviously changes the filenode and the ctid of a tuple, so we
> cannot successfully do a lookup based on what we logged before.

So I have a new idea for handling this problem, which seems obvious in
retrospect.  What if we make the VACUUM FULL or CLUSTER log the old
CTID -> new CTID mappings?  This would only need to be done for
catalog tables, and maybe could be skipped for tuples whose XIDs are
old enough that we know those transactions must already be decoded.

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

Re: logical changeset generation v6.4

From

Robert Haas

Date:

22 October 2013, 17:57:16

On Tue, Oct 22, 2013 at 10:07 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-10-21 20:16:29 +0200, Andres Freund wrote:
>> On 2013-10-18 20:50:58 +0200, Andres Freund wrote:
>> > How about modifying the selection to go from:
>> > * all rows if ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL;
>> > * index chosen by ALTER TABLE ... REPLICA IDENTITY USING indexname
>> > * [later, maybe] ALTER TABLE ... REPLICA IDENTITY (cola, colb)
>>
>> Current draft is:
>> ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL|DEFAULT
>> ALTER TABLE ... REPLICA IDENTITY USING INDEX ...;
>>
>> which leaves the door open for
>>
>> ALTER TABLE ... REPLICA IDENTITY USING '(' column_name_list ')';
>>
>> Does anybody have a strong feeling about requiring support for CREATE
>> TABLE for this?
>
> Attached is a patch ontop of master implementing this syntax. It's not
> wired up to the changeset extraction patch yet as I am not sure whether
> others agree about the storage.
>
> pg_class grew a 'relreplident' char, storing:
> * 'd' default
> * 'n' nothing
> * 'f' full
> * 'i' index with indisreplident set, or default if index has been
>       dropped
> pg_index grew a 'indisreplident' bool indicating it is set as the
> replica identity for a replident = 'i' relation.
>
> Both changes shouldn't change the width of the affected relations, they
> should reuse existing padding.
>
> Does somebody prefer a different storage?

I had imagined that the storage might consist simply of a pg_attribute
boolean.  So full would turn them all on, null would turn them all of,
etc.  But that does make it hard to implement the "whatever the pkey
happens to be right now" behavior, so maybe your idea is better.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 18:02:28

On 2013-10-22 10:52:48 -0400, Robert Haas wrote:
> On Fri, Oct 18, 2013 at 2:26 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > So. As it turns out that solution isn't sufficient in the face of VACUUM
> > FULL and mixed DML/DDL transaction that have not yet been decoded.
> >
> > To reiterate, as published it works like:
> > For every modification of catalog tuple (insert, multi_insert, update,
> > delete) that has influence over visibility issue a record that contains:
> > * filenode
> > * ctid
> > * (cmin, cmax)
> >
> > When doing a visibility check on a catalog row during decoding of mixed
> > DML/DDL transaction lookup (cmin, cmax) for that row since we don't
> > store both for the tuple.
> >
> > That mostly works great.
> >
> > The problematic scenario is decoding a transaction that has done mixed
> > DML/DDL *after* a VACUUM FULL/CLUSTER has been performed. The VACUUM
> > FULL obviously changes the filenode and the ctid of a tuple, so we
> > cannot successfully do a lookup based on what we logged before.
> 
> So I have a new idea for handling this problem, which seems obvious in
> retrospect.  What if we make the VACUUM FULL or CLUSTER log the old
> CTID -> new CTID mappings?  This would only need to be done for
> catalog tables, and maybe could be skipped for tuples whose XIDs are
> old enough that we know those transactions must already be decoded.

Ah. If it only were so simple ;). That was my first idea, and after I'd
bragged in an 2ndq internal chat that I'd found a simple idea I
obviously had to realize it doesn't work.

Consider:
INIT_LOGICAL_REPLICATION;
CREATE TABLE foo(...);
BEGIN;
INSERT INTO foo;
ALTER TABLE foo ...;
INSERT INTO foo;
COMMIT TX 3;
VACUUM FULL pg_class;
START_LOGICAL_REPLICATION;

When we decode tx 3 we haven't yet read the mapping from the vacuum
freeze. That scenario can happen either because decoding was stopped for
a moment, or because decoding couldn't keep up (slow connection,
whatever).

There also can be nasty variations where the VACUUM FULL happens while a
transaction is writing data since we don't hold locks on system
relations for very long.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

22 October 2013, 18:59:40

On Tue, Oct 22, 2013 at 11:02 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-10-22 10:52:48 -0400, Robert Haas wrote:
>> On Fri, Oct 18, 2013 at 2:26 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> > So. As it turns out that solution isn't sufficient in the face of VACUUM
>> > FULL and mixed DML/DDL transaction that have not yet been decoded.
>> >
>> > To reiterate, as published it works like:
>> > For every modification of catalog tuple (insert, multi_insert, update,
>> > delete) that has influence over visibility issue a record that contains:
>> > * filenode
>> > * ctid
>> > * (cmin, cmax)
>> >
>> > When doing a visibility check on a catalog row during decoding of mixed
>> > DML/DDL transaction lookup (cmin, cmax) for that row since we don't
>> > store both for the tuple.
>> >
>> > That mostly works great.
>> >
>> > The problematic scenario is decoding a transaction that has done mixed
>> > DML/DDL *after* a VACUUM FULL/CLUSTER has been performed. The VACUUM
>> > FULL obviously changes the filenode and the ctid of a tuple, so we
>> > cannot successfully do a lookup based on what we logged before.
>>
>> So I have a new idea for handling this problem, which seems obvious in
>> retrospect.  What if we make the VACUUM FULL or CLUSTER log the old
>> CTID -> new CTID mappings?  This would only need to be done for
>> catalog tables, and maybe could be skipped for tuples whose XIDs are
>> old enough that we know those transactions must already be decoded.
>
> Ah. If it only were so simple ;). That was my first idea, and after I'd
> bragged in an 2ndq internal chat that I'd found a simple idea I
> obviously had to realize it doesn't work.
>
> Consider:
> INIT_LOGICAL_REPLICATION;
> CREATE TABLE foo(...);
> BEGIN;
> INSERT INTO foo;
> ALTER TABLE foo ...;
> INSERT INTO foo;
> COMMIT TX 3;
> VACUUM FULL pg_class;
> START_LOGICAL_REPLICATION;
>
> When we decode tx 3 we haven't yet read the mapping from the vacuum
> freeze. That scenario can happen either because decoding was stopped for
> a moment, or because decoding couldn't keep up (slow connection,
> whatever).

That strikes me as a flaw in the implementation rather than the idea.
You're presupposing a patch where the necessary information is
available in WAL yet you don't make use of it at the proper time.  It
seems to me that you have to think of the CTID map as tied to a
relfilenode; if you try to use one relfilenode's map with a different
relfilenode, it's obviously not going to work.  So don't do that.

/me looks innocent.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 19:12:19

On 2013-10-18 20:26:16 +0200, Andres Freund wrote:
> 4) Store both (cmin, cmax) for catalog tuples.

BTW: That would have the nice side-effect of delivering the basis of
what you need to do parallel sort in a transaction that previously has
performed DDL.

Currently you cannot do anything in parallel after DDL, even if you only
scan the table in one backend, because operators et al. have to do
catalog lookups which you can't do consistently since cmin/cmax aren't
available in both.

Greetings,

Andres Freund

Re: logical changeset generation v6.2

From

Heikki Linnakangas

Date:

22 October 2013, 19:19:35

On 22.10.2013 19:12, Andres Freund wrote:
> On 2013-10-18 20:26:16 +0200, Andres Freund wrote:
>> 4) Store both (cmin, cmax) for catalog tuples.
>
> BTW: That would have the nice side-effect of delivering the basis of
> what you need to do parallel sort in a transaction that previously has
> performed DDL.
>
> Currently you cannot do anything in parallel after DDL, even if you only
> scan the table in one backend, because operators et al. have to do
> catalog lookups which you can't do consistently since cmin/cmax aren't
> available in both.

Parallel workers will need cmin/cmax for user tables too, to know which 
tuples are visible to the snapshot.

- Heikki

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 19:24:02

On 2013-10-22 19:19:19 +0300, Heikki Linnakangas wrote:
> On 22.10.2013 19:12, Andres Freund wrote:
> >On 2013-10-18 20:26:16 +0200, Andres Freund wrote:
> >>4) Store both (cmin, cmax) for catalog tuples.
> >
> >BTW: That would have the nice side-effect of delivering the basis of
> >what you need to do parallel sort in a transaction that previously has
> >performed DDL.
> >
> >Currently you cannot do anything in parallel after DDL, even if you only
> >scan the table in one backend, because operators et al. have to do
> >catalog lookups which you can't do consistently since cmin/cmax aren't
> >available in both.
> 
> Parallel workers will need cmin/cmax for user tables too, to know which
> tuples are visible to the snapshot.

The existing proposals were mostly about just parallelizing the sort and
similar operations, right? In such scenarios you really need it only for
the catalog.

But we could easily generalize it for user data too. We should even be
able to only use "wide cids" when we a backend needs it it since
inherently it's only needed within a single transaction.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Heikki Linnakangas

Date:

22 October 2013, 19:25:44

On 22.10.2013 19:23, Andres Freund wrote:
> On 2013-10-22 19:19:19 +0300, Heikki Linnakangas wrote:
>> On 22.10.2013 19:12, Andres Freund wrote:
>>> On 2013-10-18 20:26:16 +0200, Andres Freund wrote:
>>>> 4) Store both (cmin, cmax) for catalog tuples.
>>>
>>> BTW: That would have the nice side-effect of delivering the basis of
>>> what you need to do parallel sort in a transaction that previously has
>>> performed DDL.
>>>
>>> Currently you cannot do anything in parallel after DDL, even if you only
>>> scan the table in one backend, because operators et al. have to do
>>> catalog lookups which you can't do consistently since cmin/cmax aren't
>>> available in both.
>>
>> Parallel workers will need cmin/cmax for user tables too, to know which
>> tuples are visible to the snapshot.
>
> The existing proposals were mostly about just parallelizing the sort and
> similar operations, right? In such scenarios you really need it only for
> the catalog.
>
> But we could easily generalize it for user data too. We should even be
> able to only use "wide cids" when we a backend needs it it since
> inherently it's only needed within a single transaction.

Or just hand over a copy of the combocid map to the worker, along with 
the snapshot. Seems a lot simpler than this wide cids business..

- Heikki

Re: logical changeset generation v6.2

From

Robert Haas

Date:

22 October 2013, 19:30:39

On Tue, Oct 22, 2013 at 12:25 PM, Heikki Linnakangas
<hlinnakangas@vmware.com> wrote:
> Or just hand over a copy of the combocid map to the worker, along with the
> snapshot. Seems a lot simpler than this wide cids business..

Yes, that's what Noah and I talked about doing.  Or possibly even
making the map into a hash table in dynamic shared memory, so that new
combo CIDs could be allocated by any backend in the parallel group.
But that seems hard, so for starters I think we'll only parallelize
read-only operations and just hand over a copy of the map.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 19:43:59

On 2013-10-22 19:25:31 +0300, Heikki Linnakangas wrote:
> On 22.10.2013 19:23, Andres Freund wrote:
> >On 2013-10-22 19:19:19 +0300, Heikki Linnakangas wrote:
> >>On 22.10.2013 19:12, Andres Freund wrote:
> >>>On 2013-10-18 20:26:16 +0200, Andres Freund wrote:
> >>>>4) Store both (cmin, cmax) for catalog tuples.
> >>>
> >>>BTW: That would have the nice side-effect of delivering the basis of
> >>>what you need to do parallel sort in a transaction that previously has
> >>>performed DDL.
> >>>
> >>>Currently you cannot do anything in parallel after DDL, even if you only
> >>>scan the table in one backend, because operators et al. have to do
> >>>catalog lookups which you can't do consistently since cmin/cmax aren't
> >>>available in both.
> >>
> >>Parallel workers will need cmin/cmax for user tables too, to know which
> >>tuples are visible to the snapshot.
> >
> >The existing proposals were mostly about just parallelizing the sort and
> >similar operations, right? In such scenarios you really need it only for
> >the catalog.
> >
> >But we could easily generalize it for user data too. We should even be
> >able to only use "wide cids" when we a backend needs it it since
> >inherently it's only needed within a single transaction.
> 
> Or just hand over a copy of the combocid map to the worker, along with the
> snapshot. Seems a lot simpler than this wide cids business..

That's not sufficient if you want to continue writing in the primary
backend though which isn't an uninteresting thing.

I am not saying that parallel XXX is a sufficient reason for this, just
that it might a be a co-benefactor.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 20:08:53

On 2013-10-22 11:59:35 -0400, Robert Haas wrote:
> >> So I have a new idea for handling this problem, which seems obvious in
> >> retrospect.  What if we make the VACUUM FULL or CLUSTER log the old
> >> CTID -> new CTID mappings?  This would only need to be done for
> >> catalog tables, and maybe could be skipped for tuples whose XIDs are
> >> old enough that we know those transactions must already be decoded.
> >
> > Ah. If it only were so simple ;). That was my first idea, and after I'd
> > bragged in an 2ndq internal chat that I'd found a simple idea I
> > obviously had to realize it doesn't work.
> >
> > Consider:
> > INIT_LOGICAL_REPLICATION;
> > CREATE TABLE foo(...);
> > BEGIN;
> > INSERT INTO foo;
> > ALTER TABLE foo ...;
> > INSERT INTO foo;
> > COMMIT TX 3;
> > VACUUM FULL pg_class;
> > START_LOGICAL_REPLICATION;
> >
> > When we decode tx 3 we haven't yet read the mapping from the vacuum
> > freeze. That scenario can happen either because decoding was stopped for
> > a moment, or because decoding couldn't keep up (slow connection,
> > whatever).

> It seems to me that you have to think of the CTID map as tied to a
> relfilenode; if you try to use one relfilenode's map with a different
> relfilenode, it's obviously not going to work.  So don't do that.

It has to be tied to relfilenode (+ctid) *and* transaction
unfortunately.
> That strikes me as a flaw in the implementation rather than the idea.
> You're presupposing a patch where the necessary information is
> available in WAL yet you don't make use of it at the proper time.

The problem is that the mapping would be somewhere *ahead* from the
transaction/WAL we're currently decoding. We'd need to read ahead till
we find the correct one.
But I think I mainly misunderstood what you proposed. That mapping could
be written besides relfilenode, instead of into the WAL. Then my
imagined problem doesn't exist anymore.

We only would need to write out mappings for tuples modified since the
xmin horizon, so it wouldn't even be *too* bad for bigger relations.

This won't easily work for two+ rewrites because we'd need to apply all
mappings in order and thus would have to keep a history of intermediate
nodes/mappings. But it'd be perfectly doable to simply wait till
decoders are caught up.

I still "feel" that simply storing both cmin, cmax is cleaner, but if
that's not acceptable, I can certainly live with something like this.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

22 October 2013, 20:57:58

On Tue, Oct 22, 2013 at 1:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> It seems to me that you have to think of the CTID map as tied to a
>> relfilenode; if you try to use one relfilenode's map with a different
>> relfilenode, it's obviously not going to work.  So don't do that.
>
> It has to be tied to relfilenode (+ctid) *and* transaction
> unfortunately.

I agree that it does, but it doesn't seem particularly unfortunate to me.

>> That strikes me as a flaw in the implementation rather than the idea.
>> You're presupposing a patch where the necessary information is
>> available in WAL yet you don't make use of it at the proper time.
>
> The problem is that the mapping would be somewhere *ahead* from the
> transaction/WAL we're currently decoding. We'd need to read ahead till
> we find the correct one.

Yes, I think that's what you need to do.

> But I think I mainly misunderstood what you proposed. That mapping could
> be written besides relfilenode, instead of into the WAL. Then my
> imagined problem doesn't exist anymore.

That's pretty ugly.  I think it should be written into WAL.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

22 October 2013, 21:13:30

On 2013-10-22 13:57:53 -0400, Robert Haas wrote:
> On Tue, Oct 22, 2013 at 1:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> That strikes me as a flaw in the implementation rather than the idea.
> >> You're presupposing a patch where the necessary information is
> >> available in WAL yet you don't make use of it at the proper time.
> >
> > The problem is that the mapping would be somewhere *ahead* from the
> > transaction/WAL we're currently decoding. We'd need to read ahead till
> > we find the correct one.
> 
> Yes, I think that's what you need to do.

My problem with that is that rewrite can be gigabytes into the future.

When reading forward we could either just continue reading data into the
reorderbuffer, but delay replaying all future commits till we found the
currently needed remap. That might have quite the additional
storage/memory cost, but runtime complexity should be the same as normal
decoding.
Or we could individually read ahead for every transaction. But doing so
for every transaction will get rather expensive (rougly O(amount_of_wal^2)).

I think that'd be pretty similar to just disallowing VACUUM
FREEZE/CLUSTER on catalog relations since effectively it'd be to
expensive to use.

> > But I think I mainly misunderstood what you proposed. That mapping could
> > be written besides relfilenode, instead of into the WAL. Then my
> > imagined problem doesn't exist anymore.
> 
> That's pretty ugly.  I think it should be written into WAL.

It basically has O(1) access, that's why I was thinking about it.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

24 October 2013, 17:59:32

On Tue, Oct 22, 2013 at 2:13 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-10-22 13:57:53 -0400, Robert Haas wrote:
>> On Tue, Oct 22, 2013 at 1:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> >> That strikes me as a flaw in the implementation rather than the idea.
>> >> You're presupposing a patch where the necessary information is
>> >> available in WAL yet you don't make use of it at the proper time.
>> >
>> > The problem is that the mapping would be somewhere *ahead* from the
>> > transaction/WAL we're currently decoding. We'd need to read ahead till
>> > we find the correct one.
>>
>> Yes, I think that's what you need to do.
>
> My problem with that is that rewrite can be gigabytes into the future.
>
> When reading forward we could either just continue reading data into the
> reorderbuffer, but delay replaying all future commits till we found the
> currently needed remap. That might have quite the additional
> storage/memory cost, but runtime complexity should be the same as normal
> decoding.
> Or we could individually read ahead for every transaction. But doing so
> for every transaction will get rather expensive (rougly O(amount_of_wal^2)).

[ Sorry it's taken me a bit of time to get back to this; other tasks
intervened, and I also just needed some time to let it settle in my
brain. ]

If you read ahead looking for a set of ctid translations from
relfilenode A to relfilenode B, and along the way you happen to
encounter a set of translations from relfilenode C to relfilenode D,
you could stash that set of translations away somewhere, so that if
the next transaction you process needs that set of mappings, it's
already computed.  With that approach, you'd never have to pre-read
the same set of WAL files more than once.

But, as I think about it more, that's not very different from your
idea of stashing the translations someplace other than WAL in the
first place.  I mean, if the read-ahead thread generates a series of
files in pg_somethingorother that contain those maps, you could have
just written the maps to that directory in the first place.  So on
further review I think we could adopt that approach.

However, I'm leery about the idea of using a relation fork for this.
I'm not sure whether that's what you had it mind, but it gives me the
willies.  First, it adds distributed overhead to the system, as
previously discussed; and second, I think the accounting may be kind
of tricky, especially in the face of multiple rewrites.  I'd be more
inclined to find a separate place to store the mappings.  Note that,
AFAICS, there's no real need for the mapping file to be
block-structured, and I believe they'll be written first (with no
readers) and subsequently only read (with no further writes) and
eventually deleted.

One possible objection to this is that it would preclude decoding on a
standby, which seems like a likely enough thing to want to do.  So
maybe it's best to WAL-log the changes to the mapping file so that the
standby can reconstruct it if needed.

> I think that'd be pretty similar to just disallowing VACUUM
> FREEZE/CLUSTER on catalog relations since effectively it'd be to
> expensive to use.

This seems unduly pessimistic to me; unless the catalogs are really
darn big, this is a mostly theoretical problem.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

25 October 2013, 14:57:24

On 2013-10-24 10:59:21 -0400, Robert Haas wrote:
> On Tue, Oct 22, 2013 at 2:13 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2013-10-22 13:57:53 -0400, Robert Haas wrote:
> >> On Tue, Oct 22, 2013 at 1:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> >> That strikes me as a flaw in the implementation rather than the idea.
> >> >> You're presupposing a patch where the necessary information is
> >> >> available in WAL yet you don't make use of it at the proper time.
> >> >
> >> > The problem is that the mapping would be somewhere *ahead* from the
> >> > transaction/WAL we're currently decoding. We'd need to read ahead till
> >> > we find the correct one.
> >>
> >> Yes, I think that's what you need to do.
> >
> > My problem with that is that rewrite can be gigabytes into the future.
> >
> > When reading forward we could either just continue reading data into the
> > reorderbuffer, but delay replaying all future commits till we found the
> > currently needed remap. That might have quite the additional
> > storage/memory cost, but runtime complexity should be the same as normal
> > decoding.
> > Or we could individually read ahead for every transaction. But doing so
> > for every transaction will get rather expensive (rougly O(amount_of_wal^2)).
> 
> [ Sorry it's taken me a bit of time to get back to this; other tasks
> intervened, and I also just needed some time to let it settle in my
> brain. ]

No worries. I've had enough things to work on ;)

> If you read ahead looking for a set of ctid translations from
> relfilenode A to relfilenode B, and along the way you happen to
> encounter a set of translations from relfilenode C to relfilenode D,
> you could stash that set of translations away somewhere, so that if
> the next transaction you process needs that set of mappings, it's
> already computed.  With that approach, you'd never have to pre-read
> the same set of WAL files more than once.

> But, as I think about it more, that's not very different from your
> idea of stashing the translations someplace other than WAL in the
> first place.  I mean, if the read-ahead thread generates a series of
> files in pg_somethingorother that contain those maps, you could have
> just written the maps to that directory in the first place.  So on
> further review I think we could adopt that approach.

Yea, that basically was my reasoning, only expressed much more nicely ;)

> However, I'm leery about the idea of using a relation fork for this.
> I'm not sure whether that's what you had it mind, but it gives me the
> willies.  First, it adds distributed overhead to the system, as
> previously discussed; and second, I think the accounting may be kind
> of tricky, especially in the face of multiple rewrites.  I'd be more
> inclined to find a separate place to store the mappings.  Note that,
> AFAICS, there's no real need for the mapping file to be
> block-structured, and I believe they'll be written first (with no
> readers) and subsequently only read (with no further writes) and
> eventually deleted.

I was thinking of storing it along other data used during logical
decoding and let decoding's cleanup clean up that data as well. All the
information for that should be there.

There's one snag I currently can see, namely that we actually need to
prevent that a formerly dropped relfilenode is getting reused. Not
entirely sure what the best way for that is.

> One possible objection to this is that it would preclude decoding on a
> standby, which seems like a likely enough thing to want to do.  So
> maybe it's best to WAL-log the changes to the mapping file so that the
> standby can reconstruct it if needed.

The mapping file probably can be one big wal record, so it should be
easy enough to do.

For a moment I thought there's a problem with decoding on the standby
having to read ahead of the current location to find the newer mapping,
but that's actually not required since we're protected by the AEL lock
during rewrites on the standby as well.

> > I think that'd be pretty similar to just disallowing VACUUM
> > FREEZE/CLUSTER on catalog relations since effectively it'd be to
> > expensive to use.
> 
> This seems unduly pessimistic to me; unless the catalogs are really
> darn big, this is a mostly theoretical problem.

Well, it's not the size of the relation, but the amount of concurrent
WAL that's being generated that matters. But anyway, if we do it like
you described above that shouldn't be a problem.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

25 October 2013, 15:14:37

On 2013-10-21 16:15:58 +0200, Andres Freund wrote:
> > I don't think I understand exactly what you have in mind for (2); can
> > you elaborate?  I have always thought that having a
> > WaitForDecodingToCatchUp() primitive was a good way of handling
> > changes that were otherwise too difficult to track our way through.  I
> > am not sure you're doing that at all right now, which in some sense I
> > guess is fine, but I haven't really understood your aversion to this
> > solution.  There are some locking issues to be worked out here, but
> > the problems don't seem altogether intractable.
> 
> So, what we need to do for rewriting catalog tables would be:
> 1) lock table against writes
> 2) wait for all in-progress xacts to finish, they could have modified
>    the table in question (we don't keep locks on system tables)
> 3) acquire xlog insert pointer
> 4) wait for all logical decoding actions to read past that pointer
> 5) upgrade the lock to an access exclusive one
> 6) perform vacuum full as usual
> 
> The lock upgrade hazards in here are the reason I am adverse to the
> solution. And I don't see how we can avoid them, since in order for
> decoding to catchup it has to be able to read from the
> catalog... Otherwise it's easy enough to implement.

So, I thought about this for some more and I think I've a partial
solution to the problem.

The worst thing about deadlocks that occur in the above is that they
could be the VACUUM FULL waiting for the "restart LSN"[1] of a decoding
slot to progress, but the restart LSN cannot progress because the slot
is waiting for a xid/transaction to end which is being blocked by the
lock upgrade from VACUUM FULL. Such conflicts are not visible to the
deadlock detector, which obviously is bad.
I've prototyped this (~25 lines) and this happens pretty frequently. But
it turns out that we can actually fix this by exporting (to shared
memory) the oldest in-progress xid of a decoding slot. Then the waiting
code can do a XactLockTableWait() for that xid...

I wonder if this is isn't maybe sufficient. Yes, it can deadlock, but
that's already the case for VACUUM FULLs of system tables, although less
likely. And it will be detected/handled.
There's one more snag though, we currently allow CLUSTER system_table;
in an existing transaction. I think that'd have to be disallowed.

What do you think?

Greetings,

Andres Freund

[1] The "restart LSN" is the point from where we need to be able read
WAL to replay all changes the receiving side hasn't acked yet.

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

Re: logical changeset generation v6.4

From

Andres Freund

Date:

25 October 2013, 17:58:23

Hi,

On 2013-10-22 16:07:16 +0200, Andres Freund wrote:
> On 2013-10-21 20:16:29 +0200, Andres Freund wrote:
> > Current draft is:
> > ALTER TABLE ... REPLICA IDENTITY NOTHING|FULL|DEFAULT
> > ALTER TABLE ... REPLICA IDENTITY USING INDEX ...;
> > 
> > which leaves the door open for
> > 
> > ALTER TABLE ... REPLICA IDENTITY USING '(' column_name_list ')';
> > 
> > Does anybody have a strong feeling about requiring support for CREATE
> > TABLE for this?
> 
> Attached is a patch ontop of master implementing this syntax. It's not
> wired up to the changeset extraction patch yet as I am not sure whether
> others agree about the storage.

So, I am currently wondering about how to store the "old" tuple, based
on this. Currently it is stored using the TupleDesc of the index the old
tuple is based on. But if we want to allow transporting the entire tuple
that obviously cannot be the only option.
One option would be to change the stored format based on what's
configured, using the relation's TupleDesc if FULL is used. But I think
always using the heap relation's desc is better.
The not-logged columns would then just be represented as NULLs. That
will make old primary keys bigger if the relation has a high number of
columns and the key small, but I don't think it matters enough.

Opinions?

Greetings,

Andres Freund

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

Re: logical changeset generation v6.4

From

Robert Haas

Date:

26 October 2013, 05:07:56

On Fri, Oct 25, 2013 at 10:58 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> So, I am currently wondering about how to store the "old" tuple, based
> on this. Currently it is stored using the TupleDesc of the index the old
> tuple is based on. But if we want to allow transporting the entire tuple
> that obviously cannot be the only option.
> One option would be to change the stored format based on what's
> configured, using the relation's TupleDesc if FULL is used. But I think
> always using the heap relation's desc is better.

I heartily agree.

> The not-logged columns would then just be represented as NULLs. That
> will make old primary keys bigger if the relation has a high number of
> columns and the key small, but I don't think it matters enough.

Even if it does matter, the cure seems likely to be worse than the disease.

My only other comment is that if NONE is selected, we ought to omit
the old tuple altogether, not store one that is all-nulls.  But I bet
you had that in mind anyway.

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

28 October 2013, 18:54:36

On Fri, Oct 25, 2013 at 7:57 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> However, I'm leery about the idea of using a relation fork for this.
>> I'm not sure whether that's what you had it mind, but it gives me the
>> willies.  First, it adds distributed overhead to the system, as
>> previously discussed; and second, I think the accounting may be kind
>> of tricky, especially in the face of multiple rewrites.  I'd be more
>> inclined to find a separate place to store the mappings.  Note that,
>> AFAICS, there's no real need for the mapping file to be
>> block-structured, and I believe they'll be written first (with no
>> readers) and subsequently only read (with no further writes) and
>> eventually deleted.
>
> I was thinking of storing it along other data used during logical
> decoding and let decoding's cleanup clean up that data as well. All the
> information for that should be there.

That seems OK.

> There's one snag I currently can see, namely that we actually need to
> prevent that a formerly dropped relfilenode is getting reused. Not
> entirely sure what the best way for that is.

I'm not sure in detail, but it seems to me that this all part of the
same picture.  If you're tracking changed relfilenodes, you'd better
track dropped ones as well.  Completely aside from this issue, what
keeps a relation from being dropped before we've decoded all of the
changes made to its data before the point at which it was dropped?  (I
hope the answer isn't "nothing".)

>> One possible objection to this is that it would preclude decoding on a
>> standby, which seems like a likely enough thing to want to do.  So
>> maybe it's best to WAL-log the changes to the mapping file so that the
>> standby can reconstruct it if needed.
>
> The mapping file probably can be one big wal record, so it should be
> easy enough to do.

It might be better to batch it, because if you rewrite a big relation,
and the record is really big, everyone else will be frozen out of
inserting WAL for as long as that colossal record is being written and
synced.  If it's inserted in reasonably-sized chunks, the rest of the
system won't be starved as badly.

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

28 October 2013, 19:04:11

On Fri, Oct 25, 2013 at 8:14 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> So, I thought about this for some more and I think I've a partial
> solution to the problem.
>
> The worst thing about deadlocks that occur in the above is that they
> could be the VACUUM FULL waiting for the "restart LSN"[1] of a decoding
> slot to progress, but the restart LSN cannot progress because the slot
> is waiting for a xid/transaction to end which is being blocked by the
> lock upgrade from VACUUM FULL. Such conflicts are not visible to the
> deadlock detector, which obviously is bad.
> I've prototyped this (~25 lines) and this happens pretty frequently. But
> it turns out that we can actually fix this by exporting (to shared
> memory) the oldest in-progress xid of a decoding slot. Then the waiting
> code can do a XactLockTableWait() for that xid...
>
> I wonder if this is isn't maybe sufficient. Yes, it can deadlock, but
> that's already the case for VACUUM FULLs of system tables, although less
> likely. And it will be detected/handled.
> There's one more snag though, we currently allow CLUSTER system_table;
> in an existing transaction. I think that'd have to be disallowed.

It wouldn't bother me too much to restrict CLUSTER system_table by
PreventTransactionChain() at wal_level = logical, but obviously it
would be nicer if we *didn't* have to do that.

In general, I don't think waiting on an XID is sufficient because a
process can acquire a heavyweight lock without having an XID.  Perhaps
use the VXID instead?

One thought I had about waiting for decoding to catch up is that you
might do it before acquiring the lock.  Of course, you then have a
problem if you get behind again before acquiring the lock.  It's
tempting to adopt the solution we used for RangeVarGetRelidExtended,
namely: wait for catchup without the lock, acquire the lock, see
whether we're still caught up if so great else release lock and loop.
But there's probably too much starvation risk to get away with that.

On the whole, I'm leaning toward thinking that the other solution
(recording the old-to-new CTID mappings generated by CLUSTER to the
extent that they are needed) is probably more elegant.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

28 October 2013, 19:17:55

On 2013-10-28 12:04:01 -0400, Robert Haas wrote:
> On Fri, Oct 25, 2013 at 8:14 AM, Andres Freund <andres@2ndquadrant.com> wrote:

> > I wonder if this is isn't maybe sufficient. Yes, it can deadlock, but
> > that's already the case for VACUUM FULLs of system tables, although less
> > likely. And it will be detected/handled.
> > There's one more snag though, we currently allow CLUSTER system_table;
> > in an existing transaction. I think that'd have to be disallowed.
> 
> It wouldn't bother me too much to restrict CLUSTER system_table by
> PreventTransactionChain() at wal_level = logical, but obviously it
> would be nicer if we *didn't* have to do that.
> 
> In general, I don't think waiting on an XID is sufficient because a
> process can acquire a heavyweight lock without having an XID.  Perhaps
> use the VXID instead?

But decoding doesn't care about transactions that haven't "used" an XID
yet (since that means they haven't modified the catalog), so that
shouldn't be problematic.

> One thought I had about waiting for decoding to catch up is that you
> might do it before acquiring the lock.  Of course, you then have a
> problem if you get behind again before acquiring the lock.  It's
> tempting to adopt the solution we used for RangeVarGetRelidExtended,
> namely: wait for catchup without the lock, acquire the lock, see
> whether we're still caught up if so great else release lock and loop.
> But there's probably too much starvation risk to get away with that.

I think we'd pretty much always starve in that case. It'd be different
if we could detect that there weren't any writes to the table
inbetween. I can see doing that using a locking hack like autovac uses,
but brr, that'd be ugly.

> On the whole, I'm leaning toward thinking that the other solution
> (recording the old-to-new CTID mappings generated by CLUSTER to the
> extent that they are needed) is probably more elegant.

I personally still think that the "wide cmin/cmax" solution is *much*
more elegant, simpler and actually can be used for other things than
logical decoding.
Since you don't seem to agree I am going to write a prototype using such
a mapping to see how it will look though.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

28 October 2013, 19:28:52

On Mon, Oct 28, 2013 at 12:17 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> In general, I don't think waiting on an XID is sufficient because a
>> process can acquire a heavyweight lock without having an XID.  Perhaps
>> use the VXID instead?
>
> But decoding doesn't care about transactions that haven't "used" an XID
> yet (since that means they haven't modified the catalog), so that
> shouldn't be problematic.

Hmm, maybe.  But what if the deadlock has more members?  e.g. A is
blocking decoding by holding AEL w/no XID, and B is blocking A by
doing VF on a rel A needs, and decoding is blocking B.

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

29 October 2013, 17:48:09

On 2013-10-28 11:54:31 -0400, Robert Haas wrote:
> > There's one snag I currently can see, namely that we actually need to
> > prevent that a formerly dropped relfilenode is getting reused. Not
> > entirely sure what the best way for that is.
> 
> I'm not sure in detail, but it seems to me that this all part of the
> same picture.  If you're tracking changed relfilenodes, you'd better
> track dropped ones as well.

What I am thinking about is the way GetNewRelFileNode() checks for
preexisting relfilenodes. It uses SnapshotDirty to scan for existing
relfilenodes for a newly created oid. Which means already dropped
relations could be reused.
I guess it could be as simple as using SatisfiesAny (or even better a
wrapper around SatisfiesVacuum that knows about recently dead tuples).

> Completely aside from this issue, what
> keeps a relation from being dropped before we've decoded all of the
> changes made to its data before the point at which it was dropped?  (I
> hope the answer isn't "nothing".)

Nothing. But there's no need to prevent it, it'll still be in the
catalog and we don't ever access a non-catalog relation's data during
decoding.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

29 October 2013, 18:28:52

On Tue, Oct 29, 2013 at 10:47 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-10-28 11:54:31 -0400, Robert Haas wrote:
>> > There's one snag I currently can see, namely that we actually need to
>> > prevent that a formerly dropped relfilenode is getting reused. Not
>> > entirely sure what the best way for that is.
>>
>> I'm not sure in detail, but it seems to me that this all part of the
>> same picture.  If you're tracking changed relfilenodes, you'd better
>> track dropped ones as well.
>
> What I am thinking about is the way GetNewRelFileNode() checks for
> preexisting relfilenodes. It uses SnapshotDirty to scan for existing
> relfilenodes for a newly created oid. Which means already dropped
> relations could be reused.
> I guess it could be as simple as using SatisfiesAny (or even better a
> wrapper around SatisfiesVacuum that knows about recently dead tuples).

I think modifying GetNewRelFileNode() is attacking the problem from
the wrong end.  The point is that when a table is dropped, that fact
can be communicated to the same machine machinery that's been tracking
the CTID->CTID mappings.  Instead of saying "hey, the tuples that were
in relfilenode 12345 are now in relfilenode 67890 in these new
positions", it can say "hey, the tuples that were in relfilenode 12345
are now GONE".

>> Completely aside from this issue, what
>> keeps a relation from being dropped before we've decoded all of the
>> changes made to its data before the point at which it was dropped?  (I
>> hope the answer isn't "nothing".)
>
> Nothing. But there's no need to prevent it, it'll still be in the
> catalog and we don't ever access a non-catalog relation's data during
> decoding.

Oh, right.  But what about a drop of a user-catalog table?

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

Re: logical changeset generation v6.2

From

Andres Freund

Date:

29 October 2013, 18:43:36

On 2013-10-29 11:28:44 -0400, Robert Haas wrote:
> On Tue, Oct 29, 2013 at 10:47 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > On 2013-10-28 11:54:31 -0400, Robert Haas wrote:
> >> > There's one snag I currently can see, namely that we actually need to
> >> > prevent that a formerly dropped relfilenode is getting reused. Not
> >> > entirely sure what the best way for that is.
> >>
> >> I'm not sure in detail, but it seems to me that this all part of the
> >> same picture.  If you're tracking changed relfilenodes, you'd better
> >> track dropped ones as well.
> >
> > What I am thinking about is the way GetNewRelFileNode() checks for
> > preexisting relfilenodes. It uses SnapshotDirty to scan for existing
> > relfilenodes for a newly created oid. Which means already dropped
> > relations could be reused.
> > I guess it could be as simple as using SatisfiesAny (or even better a
> > wrapper around SatisfiesVacuum that knows about recently dead tuples).
>
> I think modifying GetNewRelFileNode() is attacking the problem from
> the wrong end.  The point is that when a table is dropped, that fact
> can be communicated to the same machine machinery that's been tracking
> the CTID->CTID mappings.  Instead of saying "hey, the tuples that were
> in relfilenode 12345 are now in relfilenode 67890 in these new
> positions", it can say "hey, the tuples that were in relfilenode 12345
> are now GONE".

Unfortunately I don't understand what you're suggesting. What I am
worried about is something like:

<- decoding is here
VACUUM FULL pg_class; -- rewrites filenode 1 to 2
VACUUM FULL pg_class; -- rewrites filenode 2 to 3
VACUUM FULL pg_class; -- rewrites filenode 3 to 1
<- now decode up to here

In this case there are two possible (cmin,cmax) values for a specific
tuple. One from the original filenode 1 and one for the one generated
from 3.
Now that will only happen if there's an oid wraparound which hopefully
shouldn't happen very often, but I'd like to not rely on that.

> >> Completely aside from this issue, what
> >> keeps a relation from being dropped before we've decoded all of the
> >> changes made to its data before the point at which it was dropped?  (I
> >> hope the answer isn't "nothing".)
> >
> > Nothing. But there's no need to prevent it, it'll still be in the
> > catalog and we don't ever access a non-catalog relation's data during
> > decoding.
>
> Oh, right.  But what about a drop of a user-catalog table?

Currently nothing prevents that. I am not sure it's worth worrying about
it, do you think we should?

Greetings,

Andres Freund

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

Re: logical changeset generation v6.2

From

Robert Haas

Date:

29 October 2013, 19:46:25

On Tue, Oct 29, 2013 at 11:43 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> I think modifying GetNewRelFileNode() is attacking the problem from
>> the wrong end.  The point is that when a table is dropped, that fact
>> can be communicated to the same machine machinery that's been tracking
>> the CTID->CTID mappings.  Instead of saying "hey, the tuples that were
>> in relfilenode 12345 are now in relfilenode 67890 in these new
>> positions", it can say "hey, the tuples that were in relfilenode 12345
>> are now GONE".
>
> Unfortunately I don't understand what you're suggesting. What I am
> worried about is something like:
>
> <- decoding is here
> VACUUM FULL pg_class; -- rewrites filenode 1 to 2
> VACUUM FULL pg_class; -- rewrites filenode 2 to 3
> VACUUM FULL pg_class; -- rewrites filenode 3 to 1
> <- now decode up to here
>
> In this case there are two possible (cmin,cmax) values for a specific
> tuple. One from the original filenode 1 and one for the one generated
> from 3.
> Now that will only happen if there's an oid wraparound which hopefully
> shouldn't happen very often, but I'd like to not rely on that.

Ah, OK.  I didn't properly understand the scenario you were concerned
about.  There's only a potential problem here if we get behind by more
than 4 billion relfilenodes, which seems remote, but maybe not:

http://www.pgcon.org/2013/schedule/events/595.en.html

This still seems to me to be basically an accounting problem.  At any
given time, we should *know* where the catalog tuples are located.  We
can't be decoding changes that require a given system catalog while
that system catalog is locked, so any given decoding operation happens
either before or after, not during, the rewrite of the corresponding
catalog.  As long as that VACUUM FULL operation is responsible for
updating the logical decoding metadata, we should be fine.  Any
relcache entries referencing the old relfilenode need to be
invalidated, and any CTID->[cmin,cmax] maps we're storing for those
old relfilenodes need to be invalidated, too.

>> >> Completely aside from this issue, what
>> >> keeps a relation from being dropped before we've decoded all of the
>> >> changes made to its data before the point at which it was dropped?  (I
>> >> hope the answer isn't "nothing".)
>> >
>> > Nothing. But there's no need to prevent it, it'll still be in the
>> > catalog and we don't ever access a non-catalog relation's data during
>> > decoding.
>>
>> Oh, right.  But what about a drop of a user-catalog table?
>
> Currently nothing prevents that. I am not sure it's worth worrying about
> it, do you think we should?

Maybe.  Depends partly on how ugly things get if it happens, I suppose.

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

Re: logical changeset generation v6.5

From

Andres Freund

Date:

05 November 2013, 18:21:54

Hi,

Attached to this mail and in the xlog-decoding-rebasing-remapping branch
in my git[1] repository you can find the next version of the patchset that:
* Fixes full table rewrites of catalog tables using the method Robert
  prefers (which is to log rewrite mappings to disk)
* Extract the REPLICA IDENTITY as configured with ALTER TABLE for the
  old tuple for UPDATEs and DELETEs
* Much better support for synchronous replication
* Better resource cleanup (as in we need less local WAL available)
* Lots of smaller fixes

The change around REPLICA IDENTITY is *incompatible* to older output
plugins since we now log tuples using the table's TupleDesc, not the
indexes.

Robert, I'd be very grateful if you could have a look at patch 0007
implementing what we've discussed. I kept it separate to make it easier
to look at it in isolation, but I think in the end it partially should
be merged into the wal_level=logical patch.
I still think the "wide cmin/cmax" solution is more elegant and has
wider applicability, but this works as well although it's about 5 times
the code.

Comments?

[1]: http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
Greetings,

Andres Freund

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

On 2013-11-08 17:11:58 -0500, Peter Eisentraut wrote:
> On 11/8/13, 3:03 PM, Robert Haas wrote:
> > On Fri, Nov 8, 2013 at 12:38 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> >> On Tue, Nov 5, 2013 at 10:21 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> >>> Attached to this mail and in the xlog-decoding-rebasing-remapping branch
> >>> in my git[1] repository you can find the next version of the patchset that:
> >>
> >> I have pushed patches #1 and #2 from this series as a single commit,
> >> after some editing.
> >
> > And I've also pushed patch #13, which is an almost-totally-unrelated
> > improvement that has nothing to do with logical replication, but is
> > useful all the same.
>
> Please fix this new compiler warning:
>
> pg_regress_ecpg.c: In function ‘main’:
> pg_regress_ecpg.c:170:2: warning: passing argument 3 of ‘regression_main’ from incompatible pointer type [enabled by
default]
> In file included from pg_regress_ecpg.c:19:0:
> ../../../../src/test/regress/pg_regress.h:55:5: note: expected ‘init_function’ but argument is of type ‘void
(*)(void)’

Hrmpf...

I usually run something akin to
# make -j3 -s && (cd contrib && make -j3 -s)
and then in a separate step
# make -s check-world
this is so I see compiler warnings before drowning them in check-world's
output. But ecpg/test isn't built during make in src/interfaces/ecpg,
but just during make check there.

ISTM ecpg's regression tests should be built (not run!) during
$(recurse) not just during make check. Patch towards that end attached.

Also attached is the fix for the compilation warning itself.

Greetings,

Andres Freund

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

Hi,

Changes since last version:
* fixes around the logging of toasted columns for the REPLICA IDENTITY
  in UPDATE/DELETE. Found due to a question of Robert's.
* Initial documentation for the additional wal_level, but that will
  require additional links once further patches of the series are committed.
* Comment, elog/ereport, indentation impovements in many of the patches
* Add the isolationtester tests to "make check"
  contrib/test_logical_decoding and introduce "installcheck-force" that
  forces an installcheck run even though it requires special
  configuration parameters.
* the heap rewrite checkpoint code now skips over files not named
  "map-*" instead of complaining if it cannot sscanf() the filename.
* pg_stat_logical_decoding system view: 'renamed' the numeric 'database'
  column in 'dboid' and added a join to pg_database
* Remove several FIXMEs by implementing support for dropping data of
  transactions that were running before a crash.
* Add CRC32 to snapbuild state files

Questions:
* Should we rename (INIT|START|FREE)_LOGICAL_REPLICATION into
  *_LOGICAL_DECODING?
* Should we rename FREE_LOGICAL_REPLICATION into
  STOP_LOGICAL_REPLICATION? stop_logical_replication() currently is the
  SQL level function...

Todo:
* Implement timeline handling. We need to switch timelines when extracting
  changes on a standby. I think we need to readTimeLineHistory() and
  then liOfPointInHistory() for every segment.
* Once guc and recovery.conf are merged, we might want to support using
  recovery_command to gather older wal files.

I am starting to be rather happy with the state of the patch.

01 wal_decoding: Add wal_level = logical and log data required for logical decoding
02 wal_decoding: Log xl_running_xact's at a higher frequency than checkpoints are done
03 wal_decoding: Add option to use user defined tables as catalog tables
04 wal_decoding: Introduce wal decoding via catalog timetravel
05 wal_decoding: Implement VACUUM FULL/CLUSTER support via rewrite maps
   * should probably be merged with 04, kept separate for review
06 wal_decoding: Only peg the xmin horizon for catalog tables during logical decoding
07 wal_decoding: Allow walsender's to connect to a specific database
08 wal_decoding: logical changeset extraction walsender interface
09 wal_decoding: test_decoding: Add a simple decoding module in contrib
10 wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes
11 wal_decoding: test_logical_decoding: Add extension for easier testing of logical decoding
12 wal_decoding: design document v2.4 and snapshot building design doc v0.5
13 wal_decoding: Temporarily add logical decoding regression tests to everything
   * shouldn't be committed, but it's useful for testing

Greetings,

Andres Freund

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

On 2013-11-12 19:24:39 +0100, Andres Freund wrote:
> On 2013-11-12 13:18:19 -0500, Robert Haas wrote:
> > On Tue, Nov 12, 2013 at 12:50 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> > > Completely agreed. As evidenced by the fact that the current change
> > > doesn't update all relevant comments & code. I wonder if we shouldn't
> > > leave the function the current way and just add a new function for the
> > > new behaviour.
> > > The hard thing with that would be coming up with a new
> > > name. IsSystemRelationId() having a different behaviour than
> > > IsSystemRelation() seems strange to me, so just keeping that and
> > > adapting the callers seems wrong to me.
> > > IsInternalRelation()? IsCatalogRelation()?
> >
> > Well, I went through and looked at the places that were affected by
> > this and I tend to think that most places will be happier with the new
> > definition.
>
> I agree that many if not most want the new definition.
>
> > If there are call sites that want the existing test, maybe we should
> > have IsRelationInSystemNamespace() for that, and reserve
> > IsSystemRelation() for the test as to whether it's a bona fide system
> > catalog.
>
> The big reason that I think we do not want the new behaviour for all is:
>
>  *        NB: TOAST relations are considered system relations by this test
>  *        for compatibility with the old IsSystemRelationName function.
>  *        This is appropriate in many places but not all.  Where it's not,
>  *        also check IsToastRelation.
>
> the current state of things would allow to modify toast relations in
> some places :/

So, I think I found a useful defintion of IsSystemRelation() that fixes
many of the issues with moving relations to pg_catalog: Continue to
treat all pg_toast.* relations as system tables, but only consider
initdb created relations in pg_class.
I've then added IsCatalogRelation() which has a narrower definition of
system relations, namely, it only counts toast tables if they are a
catalog's toast table.

This allows far more actions on user defined relations moved to
pg_catalog. Now they aren't stuck there anymore and can be renamed,
dropped et al. With one curious exception: We still cannot move a
relation out of pg_catalog.
I've included a hunk to allow creation of indexes on relations in
pg_catalog in heap_create(), indexes on catalog relations are prevented
way above, but maybe that should rather be a separate commit.

What do you think?

Greetings,

Andres Freund

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

Attachment

0001-Don-t-regard-user-defined-relations-in-pg_catalog-as.patch

Re: logical changeset generation v6.5

From

Peter Eisentraut

Date:

14 November 2013, 01:02:44

On 11/9/13, 5:56 AM, Andres Freund wrote:
> ISTM ecpg's regression tests should be built (not run!) during
> $(recurse) not just during make check.

Actually, I did just the opposite change some years ago.  The rationale
is, the build builds that which you want to install.

Re: logical changeset generation v6.7

From

Andres Freund

Date:

14 November 2013, 16:47:01

On 2013-11-12 18:50:33 +0100, Andres Freund wrote:
> > You've actually changed the meaning of this section (and not in a good way):
> >
> >          be set at server start. <varname>wal_level</> must be set
> > -        to <literal>archive</> or <literal>hot_standby</> to allow
> > -        connections from standby servers.
> > +        to <literal>archive</>, <literal>hot_standby</> or <literal>logical</>
> > +        to allow connections from standby servers.
> >
> > I think that the previous text meant that you needed archive - or, if
> > you want to allow connections, hot_standby.  The new text loses that
> > nuance.
>
> Yea, that's because it was lost on me in the first place...

I think that's because the nuance isn't actually in the text - note that
it is talking about max_wal_senders and talking about connections
*from*, not *to* standby servers.
I've reformulated the wal_level paragraph and used "or higher" in
several places now.

Ok, so here's a rebased version of this. I tried to fix all the issues
you mentioned, and it's based on the split off IsSystemRelation() patch,
I've sent yesterday (included here).

Greetings,

Andres Freund

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

Attachment

Re: logical changeset generation v6.7

From

Fabrízio de Royes Mello

Date:

27 November 2013, 22:39:57

On Thu, Nov 14, 2013 at 11:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>
> On 2013-11-12 18:50:33 +0100, Andres Freund wrote:
> > > You've actually changed the meaning of this section (and not in a good way):
> > >
> > > be set at server start. <varname>wal_level</> must be set
> > > - to <literal>archive</> or <literal>hot_standby</> to allow
> > > - connections from standby servers.
> > > + to <literal>archive</>, <literal>hot_standby</> or <literal>logical</>
> > > + to allow connections from standby servers.
> > >
> > > I think that the previous text meant that you needed archive - or, if
> > > you want to allow connections, hot_standby. The new text loses that
> > > nuance.
> >
> > Yea, that's because it was lost on me in the first place...
>
> I think that's because the nuance isn't actually in the text - note that
> it is talking about max_wal_senders and talking about connections
> *from*, not *to* standby servers.
> I've reformulated the wal_level paragraph and used "or higher" in
> several places now.
>
> Ok, so here's a rebased version of this. I tried to fix all the issues
> you mentioned, and it's based on the split off IsSystemRelation() patch,
> I've sent yesterday (included here).
>

Hello,

I'm trying to apply the patches but show some warnings/errors:

$ gunzip -c /home/fabrizio/Downloads/0002-wal_decoding-Add-wal_level-logical-and-log-data-requ.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644

$ gunzip -c /home/fabrizio/Downloads/0005-wal_decoding-Introduce-wal-decoding-via-catalog-time.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644

$ gunzip -c /home/fabrizio/Downloads/0006-wal_decoding-Implement-VACUUM-FULL-CLUSTER-support-v.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644

$ gunzip -c /home/fabrizio/Downloads/0007-wal_decoding-Only-peg-the-xmin-horizon-for-catalog-t.patch.gz | git apply -
warning: src/backend/access/transam/xlog.c has type 100755, expected 100644

$ gunzip -c /home/fabrizio/Downloads/0011-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz | git apply -
error: patch failed: src/bin/pg_basebackup/streamutil.c:210
error: src/bin/pg_basebackup/streamutil.c: patch does not apply

The others are applied correctly. The permission warning must be fixed and 0011 bust be rebased.

Regards,

--
Fabrízio de Royes Mello
Consultoria/Coaching PostgreSQL
>> Timbira: http://www.timbira.com.br
>> Blog sobre TI: http://fabriziomello.blogspot.com
>> Perfil Linkedin: http://br.linkedin.com/in/fabriziomello
>> Twitter: http://twitter.com/fabriziomello

Re: logical changeset generation v6.7

From

Robert Haas

Date:

29 November 2013, 05:15:25

On Thu, Nov 14, 2013 at 8:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-11-12 18:50:33 +0100, Andres Freund wrote:
>> > You've actually changed the meaning of this section (and not in a good way):
>> >
>> >          be set at server start. <varname>wal_level</> must be set
>> > -        to <literal>archive</> or <literal>hot_standby</> to allow
>> > -        connections from standby servers.
>> > +        to <literal>archive</>, <literal>hot_standby</> or <literal>logical</>
>> > +        to allow connections from standby servers.
>> >
>> > I think that the previous text meant that you needed archive - or, if
>> > you want to allow connections, hot_standby.  The new text loses that
>> > nuance.
>>
>> Yea, that's because it was lost on me in the first place...
>
> I think that's because the nuance isn't actually in the text - note that
> it is talking about max_wal_senders and talking about connections
> *from*, not *to* standby servers.
> I've reformulated the wal_level paragraph and used "or higher" in
> several places now.
>
> Ok, so here's a rebased version of this. I tried to fix all the issues
> you mentioned, and it's based on the split off IsSystemRelation() patch,
> I've sent yesterday (included here).

OK, I've committed the patch to adjust the definition of
IsSystemRelation()/IsSystemClass() and add
IsCatalogRelation()/IsCatalogClass().  I kibitzed your decision about
which function to use in a few places - specifically, I made all of
the places that cared about allow_system_table_mods uses the IsSystem
functions, and all the places that cared about invalidation messages
use the IsCatalog functions.  I don't think any of these changes are
more cosmetic, but I think it may reduce the chance of errors or
inconsistencies in the face of future changes.

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

Re: logical changeset generation v6.7

From

Robert Haas

Date:

29 November 2013, 09:16:47

On Thu, Nov 14, 2013 at 8:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> [ new patches ]

Here's an updated version of patch #2.  I didn't really like the
approach you took in the documentation, so I revised it.

Apart from that, I spent a lot of time looking at
HeapSatisfiesHOTandKeyUpdate.  I'm not very happy with your changes.
The idea seems to be that we'll iterate through all of the HOT columns
regardless, but that might be very inefficient.  Suppose there are 100
HOT columns, the last one is the only key column, and only the first
one has been modified.  Once we look at #1 and determine that it's not
HOT, we should zoom forward and skip over the next 98, and only look
at the last one; your version does not behave like that.

I think there's also some confusion in your version about what ends up
in the attnum values: they're normally adjusted by
FirstLowInvalidHeapAttributeNumber, but when bms_first_member returns
-1 then they're not.  But that's not a great thing, because -1 is
actually a valid attribute number.  I've taken a crack at rewriting
this logic, and the result looks cleaner and simpler to me, but I
haven't tested it beyond the fact that it passes make check.  See what
you think.

I haven't completely reviewed every bit of this in depth yet, but it's
1:15am, so I'm going to post what I have and throw in the towel for
tonight.

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

Attachment

logical-logging-rmh.patch

Re: logical changeset generation v6.7

From

Kyotaro HORIGUCHI

Date:

03 December 2013, 11:13:23

Hello, This is rather trivial and superficial comments as not
fully gripping functions of this patchset.

- Some patches have line offset to master. Rebase needed.

Other random comments follows,

===== 0001:
- You assined HeapTupleGetOid(tuple) into relid to read in  several points but no modification. Nevertheless, you left
HeapTupleGetOidnot replaced there. I think 'relid' just for  repeated reading has far small merit compared to demerit
of lowering readability. You'd be better to make them uniform in  either way.
 

  
===== 0002:
- You are identifying the wal_level with the expr 'wal_level >=  WAL_LEVEL_LOGICAL' but it seems somewhat improper.
- In heap_insert, you added following comment and code,
  >     * Also, if this is a catalog, we need to transmit combocids to  >     * properly decode, so log that as well.
>    */  >    need_tuple_data = RelationIsLogicallyLogged(relation);  >    if
(RelationIsAccessibleInLogicalDecoding(relation)) >        log_heap_new_cid(relation, heaptup);
 
  Actually 'is a catalog' is checkied in  RelationIsAcc...Decodeing() but this either of naming or  commnet should be
changedfor consistent look. (I this the  name of the macro is rather long but gives a vague  illustration of the
function..)

- RelationIsAccessibleInLogicalDecoding and  RelationIsLogicallyLogged are identical in code. Together with  the name
ambiguity,this is quite confising and cause of  future misuse between these macros, I suppose. Plus the names  seem too
long.
- In heap_insert, the information conveyed on rdata[3] seems to  be better to be in rdata[2] because of the scarecity
ofthe  additional information. XLOG_HEAP_CONTAINS_NEW_TUPLE only  seems to be needed. Also is in heap_multi_insert and
heap_upate.
- In heap_multi_insert, need_cids referred only once so might be  better removed.
- In heap_delete, at the point adding replica identity, same to  the aboves, rdata[3] could be moved into rdata[2]
makingnew  type like 'xl_heap_replident'.
 
- In heapam_xlog.h, the new type xl_heap_header_len is  inadequate in both of naming which is confising and
constructionon which the header in xl_heap_header is no  longer be a header and indecisive member name 't_len'..
 
- In heapam_xlog.h, XLOG_HEAP_CONTAINS_OLD looks incomplete. And  it seems to be used in nowhere in this patchset. It
shouldbe  removed.
 
- log_heap_new_cid() is called at several part just before other  xlogs is being inserted. I suppose this should be
builtin the  target xlog structures.
 
- in RecovoerPreparedTransactions(), any commend needed for the  reason calling XLogLogicalInfoActive()..
- In xact.c, the comment for the member 'didLogXid' in  TransactionStateData seems differ from it's meaning. It
becomestrue when any WAL record for the current transaction  id just has been written to WAL buffer. So the comment,
 
  > /* has xid been included in WAL record? */
  would be better be something like (Should need corrected as  I'm not native speaker.)
   /* Any WAL record for this transaction has been emitted ? */
  and also the member name should be something like  XidIsLogged. (Not so chaned?)
- The name of the function MarkCurrentTransactionIdLoggedIfAny,  although irregular abbreviations are discouraged,
seemstoo  long. Isn't MarkCur(r/rent)XidLoggedIfAny sufficient?  Anyway,  the work involving this function seems would
bebetter to be  done in some other way..
 
- The comment for RelationGetIndexAttrBitmap() should be edited  for attrKind.
- The macro name INDEX_ATTR_BITMAP_KEY should be  INDEX_ATTR_BITMAP_FKEY. And INDEX_ATTR_BITMAP_IDENTITY_KEY  should be
INDEX_ATTR_BITMAP_REPLID_KEYor something.
 
- In rel.h the member name 'rd_idattr' would be better being  'rd_replidattr' or something like that.

===== 0004:
- Could the macro name 'RelationIsUsedAsCatalogTable' be as  simple as IsUserCatalogRelation or something? It's from
the viewpoint of not only simplicity but also similarity to other  macro and/or functions having closer functionality.
You already call the table 'user_catalog_table' in rel.h.
 

To be continued to next mail.

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: logical changeset generation v6.7

From

Kyotaro HORIGUCHI

Date:

03 December 2013, 13:16:11

Hello, this is continued comments.

> ===== 0004:
....
> To be continued to next mail.


===== 0005:
- In heapam.c, it seems to be better replacing t_self only  during logical decoding.
- In GetOldestXmin(), the parameter name 'alreadyLocked' would  be better being simplly 'nolock' since alreadyLocked
seemsto  me suggesting that it will unlock the lock acquired beforehand.
 
- Before that, In LogicalDecodingAcquireFreeSlot, lock window  for procarray is extended after GetOldestXmin, but
procarray does not seem to be accessed during the additional period. If  you are holding this lock for the purpose
otherthan accessing  procArray, it'd be better to provide its own lock object.
 
   > LWLockAcquire(ProcArrayLock, LW_SHARED);   > slot->effective_xmin = GetOldestXmin(true, true, true);   >
slot->xmin= slot->effective_xmin;   >    > if (!TransactionIdIsValid(LogicalDecodingCtl->xmin) ||   >
NormalTransactionIdPrecedes(slot->effective_xmin,LogicalDecodingCtl->xmin))   >     LogicalDecodingCtl->xmin =
slot->effective_xmin;  > LWLockRelease(ProcArrayLock);
 
- In dropdb in dbcommands.c, ereport is invoked referring the  result of LogicalDecodingCountDBSlots. But it seems
betterto  me issueing this exception within LogicalDecodingCountDBSlots  even if goto is required.
 
- In LogStandbySnapshot in standby.c, two complementary  conditions are imposed on two same unlocks. It might be
somewhatparanoic but it is safer being like follows,
 
   | XLogRecPtr  recptr = InvalidXLogRecPtr;   | ....   |   | /* LogCurrentRunningXacts shoud be done before unlock
whenlogical decoding*/    | if (wal_level >= WAL_LEVEL_LOGICAL)   |    recptr = LogCurrentRunningXacts(running);   |
|LWLockRelease(ProcArrayLock);   |    | if (recptr == InvalidXLogRecPtr)   |    recptr =
LogCurrentRunningXacts(running);    - In tqual.c, in Setup/RevertFrom DecodingSnapshots, the name  CatalogSnapshotData
lookslacking unity with other  Snapshot*Data's.
 

===== 0007:
- In heapam.c, the new global variable 'RecentGlobalDataXmin' is  quite similar to 'RecentGlobalXmin' and does not
represents what it is. The name should be  changed. RecentGlobalNonCatalogXmin would be more preferable..
 
- Althgough simplly from my teste, the following part in  heapam.c
   > if (IsSystemRelation(scan->rs_rd)   >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))   >
heap_page_prune_opt(scan->rs_rd,buffer, RecentGlobalXmin);   > else   >     heap_page_prune_opt(scan->rs_rd, buffer,
RecentGlobalDataXmin);
  would be readable to be like,
   > if (IsSystemRelation(scan->rs_rd)   >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))   >   xmin =
RecentGlobalXmin  > else   >   xmin = RecentGlobalDataXmin   >     heap_page_prune_opt(scan->rs_rd, buffer, xmin);
 
   index_fetch_heap in indexam.c has similar part to this, and   you coded in latter style in IndexBuildHeapScan in
index.c.
- In procarray.c, you should add documentation for new parameter  'systable' for GetOldestXmin. And the name 'systable'
seems somewhat confusing, since its full-splled meaning is  'including systables'. This name should be changed to
'include_systable'or 'only_usertable' with inverting or  something..
 

0008 and after to come later..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: logical changeset generation v6.7

From

Andres Freund

Date:

03 December 2013, 16:18:59

On 2013-11-29 01:16:39 -0500, Robert Haas wrote:
> On Thu, Nov 14, 2013 at 8:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> > [ new patches ]
> 
> Here's an updated version of patch #2.  I didn't really like the
> approach you took in the documentation, so I revised it.

Fair enough.

> Apart from that, I spent a lot of time looking at
> HeapSatisfiesHOTandKeyUpdate.  I'm not very happy with your changes.
> The idea seems to be that we'll iterate through all of the HOT columns
> regardless, but that might be very inefficient.  Suppose there are 100
> HOT columns, the last one is the only key column, and only the first
> one has been modified.  Once we look at #1 and determine that it's not
> HOT, we should zoom forward and skip over the next 98, and only look
> at the last one; your version does not behave like that.

Well, the hot bitmap will only contains indexed columns, so for that's
only going to happen if there's actually indexes over all those
columns. And in that case it seems unlikely that the performance
of that routine matters.
That said, keeping the old performance characteristics seems like a good
idea to me. Not sure anymore why I changed it that way.

>  I've taken a crack at rewriting
> this logic, and the result looks cleaner and simpler to me, but I
> haven't tested it beyond the fact that it passes make check.  See what
> you think.

Hm. I think it actually will not abort early in call cases either, but
that looks fixable. Imagine what happens if id_attrs or key_attrs is
empty, ISTM that we'll check all hot columns in that case.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.7

From

Andres Freund

Date:

03 December 2013, 16:24:59

Hi,

On 2013-11-28 21:15:18 -0500, Robert Haas wrote:
> OK, I've committed the patch to adjust the definition of
> IsSystemRelation()/IsSystemClass() and add
> IsCatalogRelation()/IsCatalogClass().

Thanks for taking care of this!

>  I kibitzed your decision about
> which function to use in a few places - specifically, I made all of
> the places that cared about allow_system_table_mods uses the IsSystem
> functions, and all the places that cared about invalidation messages
> use the IsCatalog functions.  I don't think any of these changes are
> more cosmetic, but I think it may reduce the chance of errors or
> inconsistencies in the face of future changes.

Agreed.

Do you think we need to do anything about the
ERROR:  cannot remove dependency on schema pg_catalog because it is a system object
thingy? Imo the current state is much more consistent than the earlier
one, but that's still a quite surprising leftover...

Greetings,

Andres Freund

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

Re: logical changeset generation v6.7

From

Robert Haas

Date:

03 December 2013, 23:13:34

On Tue, Dec 3, 2013 at 8:24 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-11-28 21:15:18 -0500, Robert Haas wrote:
>> OK, I've committed the patch to adjust the definition of
>> IsSystemRelation()/IsSystemClass() and add
>> IsCatalogRelation()/IsCatalogClass().
>
> Thanks for taking care of this!
>
>>  I kibitzed your decision about
>> which function to use in a few places - specifically, I made all of
>> the places that cared about allow_system_table_mods uses the IsSystem
>> functions, and all the places that cared about invalidation messages
>> use the IsCatalog functions.  I don't think any of these changes are
>> more cosmetic, but I think it may reduce the chance of errors or
>> inconsistencies in the face of future changes.
>
> Agreed.
>
> Do you think we need to do anything about the
> ERROR:  cannot remove dependency on schema pg_catalog because it is a system object
> thingy? Imo the current state is much more consistent than the earlier
> one, but that's still a quite surprising leftover...

I don't feel obliged to change it, but I also don't see a reason not
to clean it up.

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

Re: logical changeset generation v6.7

From

Robert Haas

Date:

03 December 2013, 23:19:31

On Tue, Dec 3, 2013 at 8:18 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>>  I've taken a crack at rewriting
>> this logic, and the result looks cleaner and simpler to me, but I
>> haven't tested it beyond the fact that it passes make check.  See what
>> you think.
>
> Hm. I think it actually will not abort early in call cases either, but
> that looks fixable. Imagine what happens if id_attrs or key_attrs is
> empty, ISTM that we'll check all hot columns in that case.

Yeah, you're right.  I think the current logic will terminate when all
flags are set to false or all attribute numbers have been checked, but
it doesn't know that if HOT's been disproven then we needn't consider
further HOT columns.  I think the way to fix that is to tweak this
part:

+               if (next_hot_attnum > FirstLowInvalidHeapAttributeNumber)                       check_now =
next_hot_attnum;
+               else if (next_key_attnum > FirstLowInvalidHeapAttributeNumber)
+                       check_now = next_key_attnum;
+               else if (next_id_attnum > FirstLowInvalidHeapAttributeNumber)
+                       check_now = next_id_attnum;               else
+                       break;

What I think we ought to do there is change each of those criteria to
say if (hot_result && next_hot_attnum >
FirstLowInvalidHeapAttributeNumber) and similarly for the other two.
That way we consider each set a valid source of attribute numbers only
until the result flag for that set flips false.

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

Re: logical changeset generation v6.7

From

Andres Freund

Date:

04 December 2013, 03:42:06

Hi,

On 2013-12-03 17:13:05 +0900, Kyotaro HORIGUCHI wrote:
> - Some patches have line offset to master. Rebase needed.

Will send the rebased version as soon as I've addressed your comments.

> ===== 0001:
> 
>  - You assined HeapTupleGetOid(tuple) into relid to read in
>    several points but no modification. Nevertheless, you left
>    HeapTupleGetOid not replaced there. I think 'relid' just for
>    repeated reading has far small merit compared to demerit of
>    lowering readability. You'd be better to make them uniform in
>    either way.

It's primarily to get the line lengths halfway under control.

> ===== 0002:
> 
>  - You are identifying the wal_level with the expr 'wal_level >=
>    WAL_LEVEL_LOGICAL' but it seems somewhat improper.

Hm. Why?

>  - RelationIsAccessibleInLogicalDecoding and
>    RelationIsLogicallyLogged are identical in code. Together with
>    the name ambiguity, this is quite confising and cause of
>    future misuse between these macros, I suppose. Plus the names
>    seem too long.

Hm, don't think they are equivalent, rather the contrary. Note one
returns false if IsCatalogRelation(), the other true.

>  - In heap_insert, the information conveyed on rdata[3] seems to
>    be better to be in rdata[2] because of the scarecity of the
>    additional information. XLOG_HEAP_CONTAINS_NEW_TUPLE only
>    seems to be needed. Also is in heap_multi_insert and
>    heap_upate.

Could you explain a bit more what you mean by that? The reason it's a
separate rdata entry is that otherwise a full page write will remove the
information.

>  - In heap_multi_insert, need_cids referred only once so might be
>    better removed.

It's accessed in a loop over potentially quite some items, that's why I
moved it into an extra variable.

>  - In heap_delete, at the point adding replica identity, same to
>    the aboves, rdata[3] could be moved into rdata[2] making new
>    type like 'xl_heap_replident'.

Hm. I don't think that'd be a good idea, because we'd then need special
case decoding code for deletes because the wal format would be different
for inserts/updates and deletes.

>  - In heapam_xlog.h, the new type xl_heap_header_len is
>    inadequate in both of naming which is confising and
>    construction on which the header in xl_heap_header is no
>    longer be a header and indecisive member name 't_len'..

The "header" bit in the name refers to the fact that it's containing
information about the a HeapTuple's header, not that it's a header
itself. Do you have a better suggestion than xl_heap_header_len?

>  - In heapam_xlog.h, XLOG_HEAP_CONTAINS_OLD looks incomplete. And
>    it seems to be used in nowhere in this patchset. It should be
>    removed.

Not sure what you mean with incomplete? It contains the both possible
variants for an old contained tuple. The macro is used in the decoding,
but I don't think things get clearer if we revise the macros in that
later patch.

>  - log_heap_new_cid() is called at several part just before other
>    xlogs is being inserted. I suppose this should be built in the
>    target xlog structures.

Proportionally it will only be logged in a absolute minority of the
cases (since normally the catalog will only seldomly be updated in
comparison to a user's tables), so it doesn't seem like a good idea to
complicate the already *horribly* complicated wal format for heap_*.

>  - in RecovoerPreparedTransactions(), any commend needed for the
>    reason calling XLogLogicalInfoActive()..

It's pretty much the "Test here must match one used in
AssignTransactionId()" comment. We only want to allow overwriting if
AssignTransactionId() might already have done the SubTransSetParent()
calls.

>  - In xact.c, the comment for the member 'didLogXid' in
>    TransactionStateData seems differ from it's meaning. It
>    becomes true when any WAL record for the current transaction
>    id just has been written to WAL buffer. So the comment,
> 
>    > /* has xid been included in WAL record? */
> 
>    would be better be something like (Should need corrected as
>    I'm not native speaker.)

>     /* Any WAL record for this transaction has been emitted ? */

I don't think that'd be an improvement, transaction is a bit ambigiuous
there because it might be the toplevel or subtransaction.

>    and also the member name should be something like
>    XidIsLogged. (Not so chaned?)

Hm.

>  - The name of the function MarkCurrentTransactionIdLoggedIfAny,
>    although irregular abbreviations are discouraged, seems too
>    long. Isn't MarkCur(r/rent)XidLoggedIfAny sufficient?

If you look at the other names in xact.h that doesn't seem to fit too
well in the naming pattern.

> Anyway,
>    the work involving this function seems would be better to be
>    done in some other way..

Why? How?

>  - The comment for RelationGetIndexAttrBitmap() should be edited
>    for attrKind.

Good point.

>  - The macro name INDEX_ATTR_BITMAP_KEY should be
>    INDEX_ATTR_BITMAP_FKEY. And INDEX_ATTR_BITMAP_IDENTITY_KEY
>    should be INDEX_ATTR_BITMAP_REPLID_KEY or something.

But INDEX_ATTR_BITMAP_KEY isn't just about foreign keys... But I agree
that INDEX_ATTR_BITMAP_IDENTITY_KEY should be renamed.

Greetings,

Andres Freund

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

Re: logical changeset generation v6.7

From

Andres Freund

Date:

04 December 2013, 03:57:16

On 2013-12-03 19:15:53 +0900, Kyotaro HORIGUCHI wrote:
>  - In heapam.c, it seems to be better replacing t_self only
>    during logical decoding.

I don't see what'd be gained by that except make the test matrix bigger
at no gain.

>  - Before that, In LogicalDecodingAcquireFreeSlot, lock window
>    for procarray is extended after GetOldestXmin, but procarray
>    does not seem to be accessed during the additional period. If
>    you are holding this lock for the purpose other than accessing
>    procArray, it'd be better to provide its own lock object.

The comment above the part explains the reason:
/* * Acquire the current global xmin value and directly set the logical xmin * before releasing the lock if necessary.
Wedo this so wal decoding is * guaranteed to have all catalog rows produced by xacts with an xid > * walsnd->xmin
available.* * We can't use ComputeLogicalXmin here as that acquires ProcArrayLock * separately which would open a short
windowfor the global xmin to * advance above walsnd->xmin. */

>  - In dropdb in dbcommands.c, ereport is invoked referring the
>    result of LogicalDecodingCountDBSlots. But it seems better to
>    me issueing this exception within LogicalDecodingCountDBSlots
>    even if goto is required.

What if LogicalDecodingCountDBSlots() is needed in other places? That
seems like a layering violation to me.

>  - In LogStandbySnapshot in standby.c, two complementary
>    conditions are imposed on two same unlocks. It might be
>    somewhat paranoic but it is safer being like follows,

I don't see an advantage in that.

>  - In tqual.c, in Setup/RevertFrom DecodingSnapshots, the name
>    CatalogSnapshotData looks lacking unity with other
>    Snapshot*Data's.

That part needs a bit of work, agreed.

> ===== 0007:
> 
>  - In heapam.c, the new global variable 'RecentGlobalDataXmin' is
>    quite similar to 'RecentGlobalXmin' and does not represents
>    what it is. The name should be
>    changed. RecentGlobalNonCatalogXmin would be more preferable..

Hm. It's a mighty long name... but it indeed is clearer.

>  - Althgough simplly from my teste, the following part in
>    heapam.c
> 
>     > if (IsSystemRelation(scan->rs_rd)
>     >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))
>     >     heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalXmin);
>     > else
>     >     heap_page_prune_opt(scan->rs_rd, buffer, RecentGlobalDataXmin);
> 
>    would be readable to be like,
> 
>     > if (IsSystemRelation(scan->rs_rd)
>     >     || RelationIsAccessibleInLogicalDecoding(scan->rs_rd))
>     >   xmin = RecentGlobalXmin
>     > else
>     >   xmin = RecentGlobalDataXmin
>     >     heap_page_prune_opt(scan->rs_rd, buffer, xmin);

Well, it requires introducing a new variable (which better not be named
xmin, but OldestXmin or similar). But I don't really care.

>     index_fetch_heap in indexam.c has similar part to this, and
>     you coded in latter style in IndexBuildHeapScan in index.c.

It's different there, because we do an explicit GetOldestXmin() call
there which we surely don't want to do twice.

> 0008 and after to come later..

Thanks for your review!

Greetings,

Andres Freund

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

Re: logical changeset generation v6.7

From

Kyotaro HORIGUCHI

Date:

04 December 2013, 11:32:09

Hello, this is cont'd comments.

> 0008 and after to come later..

I had nothing to comment for patch 0008.

===== 0009: 
- In repl_scanner.l, you omitted double-doublequote handling for  replication but it should be implemented. Zero-length
identifier check might be needed depending on the upper-layer.
 
- In walsender.c, the log messages "Initiating logical rep.."  and "Starting logical replication.." should be INFO or
LOGin  loglevel, not WARNING. And 'rep' in the former message would  be better not abbreviated since not done so in the
latter.
- In walsender.c, StartLogicalReplication seems trying to abort  itself for timeline change. But timeline changes in
9.3+don't  need such an aid. You'd better consult StartReplication in  current master for detail. There might be other
defferences.
- In walsender.c, the typedef name WalSndSendData doesn't seem  to be a function pointer. I suppose passing bare
function pointer to WanSndLoop and WalSndDone is not a good deed. It'd  be better to wrap it in any struct for
callback,say,  LogicalDecodingContext. It'd be even better if it could be a  common struct with 'physycal'
replication.
- In walsender.c, I wonder if the differences are necessary  between logical and physical replication in fetching
latest WALs, construction of WAL sending loop and so on .. Logical  walsender seems to be implimentated in somewhat
ad-hocway on  the whole. I belive it could be more commonize in the base  structure.
 
- In procarray.c, the added two includes which is not  accompanied by any other modification are needless. make emits
noerror or warning without them.
 

...Time's up. It'll be continued for later from 0010..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center

Re: logical changeset generation v6.7

From

Andres Freund

Date:

04 December 2013, 11:54:26

On 2013-12-04 17:31:50 +0900, Kyotaro HORIGUCHI wrote:
> ===== 0009:
>
>  - In repl_scanner.l, you omitted double-doublequote handling for
>    replication but it should be implemented. Zero-length
>    identifier check might be needed depending on the upper-layer.

I am not sure what you mean here. IDENT can be double quoted, and so can
the option names?

>  - In walsender.c, the log messages "Initiating logical rep.."
>    and "Starting logical replication.." should be INFO or LOG in
>    loglevel, not WARNING. And 'rep' in the former message would
>    be better not abbreviated since not done so in the latter.

Agreed.

>  - In walsender.c, StartLogicalReplication seems trying to abort
>    itself for timeline change. But timeline changes in 9.3+ don't
>    need such an aid. You'd better consult StartReplication in
>    current master for detail. There might be other defferences.

Timeline increases currently need work, yes, that error messgage is the
smallest part...

>  - In walsender.c, the typedef name WalSndSendData doesn't seem
>    to be a function pointer. I suppose passing bare function
>    pointer to WanSndLoop and WalSndDone is not a good deed. It'd
>    be better to wrap it in any struct for callback, say,
>    LogicalDecodingContext. It'd be even better if it could be a
>    common struct with 'physycal' replication.

I don't see that as being realistic/advantageous. Wrapping a function
pointer in a struct doesn't improve anything in itself.

I was thinking we might want to just decouple the entire event loop and
not reuse that code, but that's ugly as well.

>  - In walsender.c, I wonder if the differences are necessary
>    between logical and physical replication in fetching latest
>    WALs, construction of WAL sending loop and so on .. Logical
>    walsender seems to be implimentated in somewhat ad-hoc way on
>    the whole. I belive it could be more commonize in the base
>    structure.

That's because the xlogreader.h interface - over my loud protests -
doesn't support chunk-wise reading of the WAL stream and neccessicates
blocking inside the reader callback. So the event loop needs to be in
several individual functions (WalSndLoop, WalSndWaitForWal,
WalSndWriteData) instead of once in WalSndLoop…

>  - In procarray.c, the added two includes which is not
>    accompanied by any other modification are needless. make emits
>    no error or warning without them.

Right. Will remove.

Thanks,

Andres Freund

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

Re: logical changeset generation v6.8

From

Andres Freund

Date:

04 December 2013, 18:55:23

On 2013-12-03 15:19:26 -0500, Robert Haas wrote:
> Yeah, you're right.  I think the current logic will terminate when all
> flags are set to false or all attribute numbers have been checked, but
> it doesn't know that if HOT's been disproven then we needn't consider
> further HOT columns.  I think the way to fix that is to tweak this
> part:
>
> +               if (next_hot_attnum > FirstLowInvalidHeapAttributeNumber)
>                         check_now = next_hot_attnum;
> +               else if (next_key_attnum > FirstLowInvalidHeapAttributeNumber)
> +                       check_now = next_key_attnum;
> +               else if (next_id_attnum > FirstLowInvalidHeapAttributeNumber)
> +                       check_now = next_id_attnum;
>                 else
> +                       break;
>
> What I think we ought to do there is change each of those criteria to
> say if (hot_result && next_hot_attnum >
> FirstLowInvalidHeapAttributeNumber) and similarly for the other two.
> That way we consider each set a valid source of attribute numbers only
> until the result flag for that set flips false.

That seems to work well, yes.

Updated & rebased series attached.

* Rebased since the former patch 01 has been applied
* Lots of smaller changes in the wal_level=logical patch
  * Use Robert's version of wal_level=logical, with the above fixes
  * Use only macros for RelationIsAccessibleInLogicalDecoding/LogicallyLogged
  * Moved a mit more logic into ExtractReplicaIdentity
  * some comment copy-editing
  * Bug noted by Euler fixed, testcase added
* Some copy editing in later patches, nothing significant.

I've primarily sent this, because I don't know of further required
changes in 0001-0003. I am trying reviewing the other patches in detail
atm.

Greetings,

Andres Freund

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

On 2013-12-10 19:11:03 -0500, Robert Haas wrote:
> Committed #1 (again).  Regarding this:
>
> +       /* XXX: we could also do this unconditionally, the space is used anyway
> +       if (copy_oid)
> +               HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp));
>
> I would like to put in a big +1 for doing that unconditionally.  I
> didn't make that change before committing, but I think it'd be a very
> good idea.

Patch attached.

Greetings,

Andres Freund

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

Attachment

0001-Dep-t-of-second-thoughts-Always-include-oids-in-WAL-.patch

Re: logical changeset generation v6.8

From

Robert Haas

Date:

11 December 2013, 21:46:05

On Wed, Dec 11, 2013 at 11:25 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-12-10 19:11:03 -0500, Robert Haas wrote:
>> Committed #1 (again).  Regarding this:
>>
>> +       /* XXX: we could also do this unconditionally, the space is used anyway
>> +       if (copy_oid)
>> +               HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp));
>>
>> I would like to put in a big +1 for doing that unconditionally.  I
>> didn't make that change before committing, but I think it'd be a very
>> good idea.
>
> Patch attached.

Committed with kibitzing.

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

Re: logical changeset generation v6.8

From

Andres Freund

Date:

12 December 2013, 03:08:44

Hi,

Lots of sensible comments removed, I plan to make changes to
address them.

On 2013-12-10 22:17:44 -0500, Robert Haas wrote:
> - I think this needs SGML documentation, same kind of thing we have
> for background workers, except probably significantly more.  A design
> document with ASCII art in a different patch does not constitute
> usable documentation.  I think it would fit best under section VII,
> internals, perhaps entitled "Writing a Logical Decoding Plugin".
> Looking at it, I rather wonder if the "Background Worker Processes"
> ought to be there as well, rather than under section V, server
> programming.

Completely agreed it needs that. I'd like the UI decisions in
http://www.postgresql.org/message-id/20131205001520.GA8935@awork2.anarazel.de
resolved before though.

> +       logical_xmin =
> +               ((volatile LogicalDecodingCtlData*) LogicalDecodingCtl)->xmin;
> 
> Ugly.

We really should have a macro for this...

> Message style.  I suggest treating a short write as ENOSPC; there is
> precedent elsewhere.
> 
> I don't think there's much point in including "remapping" in all of
> the error messages.  It adds burden for translators and users won't
> know what a remapping file is anyway.

It helps in locating wich part of the code caused a problem. I utterly
hate to get reports with error messages that I can't correlate to a
sourcefile. Yes, I know that verbose error output exists, but usually
you don't get it that way... That said, I'll try to make the messages
simpler.

> +       /* use *GetUpdateXid to correctly deal with multixacts */
> +       xmax = HeapTupleHeaderGetUpdateXid(new_tuple->t_data);
>
> I don't feel enlightened by that comment.

Well, it will return the update xid of a tuple locked with NO KEY SHARE
and updated (with NO KEY UPDATE). I.e. 9.3+ foreign key locking stuff.

> +       if (!TransactionIdIsNormal(xmax))
> +       {
> +               /*
> +                * no xmax is set, can't have any permanent ones, so
> this check is
> +                * sufficient
> +                */
> +       }
> +       else if (HEAP_XMAX_IS_LOCKED_ONLY(new_tuple->t_data->t_infomask))
> +       {
> +               /* only locked, we don't care */
> +       }
> +       else if (!TransactionIdPrecedes(xmax, cutoff))
> +       {
> +               /* tuple has been deleted recently, log */
> +               do_log_xmax = true;
> +       }
> 
> Obfuscated.  Rewrite without empty blocks.

I don't understand why having an empty block is less clear than
including a condition in several branches? Especially if the individual
conditions might need to be documented?

> +       /*
> +        * Write out buffer everytime we've too many in-memory entries.
> +        */
> +       if (state->rs_num_rewrite_mappings >= 1000 /* arbitrary number */)
> +               logical_heap_rewrite_flush_mappings(state);
> 
> What happens if the number of items per hash table entry is small but
> the number of entries is large?

rs_num_rewrite_mappings is the overall number of in-memory mappings, not
the number of per-entry mappings. That means we flush, even if all
entries have only a couple of mappings, as soon as 1000 in memory
entries have been collected. A bit simplistic, but seems okay enough?

> +               /* XXX: should we warn about such files? */
> 
> Nah.

Ok, will remove that comment from a couple locations then...

> +                       /* XXX: we could replay the transaction and
> prepare it as well. */
> 
> Should we do that?

It would allow a neat feature, namely using 2PC to make sure that a
transaction commit on all the nodes connected using changeset
extraction. But I think that's a feature for later. Its use would have
to be optional anyway.

> All right, I'm giving up for now.  These patches need a LOT of work on
> comments and documentation to be committable, even if the underlying
> architecture is basically sound.

> +        * clog. If we're doing logical replication we can't do that though, so
> +        * hold the lock for a moment longer.
> 
> ...because why?

That's something pretty icky imo. But more in the existing HS code than
in this. Without the changes in the locking we can have the situation
that transactions are marked as running in the xl_running_xact record,
but are actually already committed. There's some code for HS that tries
to cope with that situation but I don't trust it very much, and it'd be
more complicated to make it work for logical decoding. I could reproduce
problems for the latter without those changes.

I'll add a comment explaining this.

> I'm still unhappy that we're introducing logical decoding slots but no
> analogue for physical replication.  If we had the latter, would it
> share meaningful amounts of structure with this?

Yes, I think we could mostly reuse it, we'd probably want to add a field
or two more (application_name, sync_prio?). I have been wondering
whether some of the code in replication/logical/logical.c shouldn't be
in replication/slot.c or similar. So far I've opted for leaving it in
its current place since it would have to change a bit for a more general
role.

I still think that the technical parts of properly supporting persistent
slots for physical rep aren't that hard, but that the behavioural
decisions are. I think there are primarily two things for SR that we
want to prevent using slots:
a) removal of still used WAL (i.e. maintain knowledge about a standby's  last required REDO location)
b) make hot_standby_feedback work across disconnections of the walsender  connection (i.e peg xmin, not just for
catalogsthough)

c) Make sure we can transport those across cascading  replication.
once those are there it's also useful to keep a bit more information
about the state of replicas:
* write, flush, apply lsn

The hard questions that I see are like:
* How do we manage standby registration? Does the admin have to do that manually? Or does a standby register itself
automaticallyif some config paramter is set?

* If automatically, how do we deal with the situation that registrant dies before noting his own identifier somewhere
persistent?My best idea is a two phase registration process where registration in phase 1 are thrown away after a
restart,but yuck.

* How do we deal with the usability wart that people *will* forget to delete a slot when removing a node?
* What requirements do we have for transporting knowlede about this across a failover?

I have little hope that we can resolve all that for 9.4.

> + * noncompatible way, but those are prevented both on catalog + *
> tables and on user tables declared as additional catalog + * tables.
> 
> Really?

Yes, I think so, c.f. ATRewriteTables():        /*         * We don't support rewriting of system catalogs; there are
too        * many corner cases and too little benefit.  In particular this         * is certainly not going to work for
mappedcatalogs.         */        if (IsSystemRelation(OldHeap))            ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),                    errmsg("cannot rewrite system relation \"%s\"",
              RelationGetRelationName(OldHeap))));

        if (RelationIsUsedAsCatalogTable(OldHeap))            ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),                    errmsg("cannot rewrite table \"%s\" used as a catalog
table",                           RelationGetRelationName(OldHeap))));

Do you see situations where that's not sufficient? It's not even
dependent on allow_system_table_mods ...

> My eyes are starting to glaze over, so really stopping here.

There's quite a bit to do from that point, so I think you've more than
done your duty... Thanks!

Greetings,

Andres Freund

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

Re: logical changeset generation v6.8

From

David Rowley

Date:

13 December 2013, 10:58:29

On Wed, Dec 11, 2013 at 1:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:

Committed #1 (again). Regarding this:

This introduced a new compiler warning on the visual studios build:

d:\postgres\b\src\backend\utils\cache\relcache.c(3958): warning C4715: 'RelationGetIndexAttrBitmap' : not all control paths return a value [D:\Postgres\b\postgres.vcxproj]

The attached patch fixes it.

Regards

David Rowley

+ /* XXX: we could also do this unconditionally, the space is used anyway
+ if (copy_oid)
+ HeapTupleSetOid(key_tuple, HeapTupleGetOid(tp));

I would like to put in a big +1 for doing that unconditionally. I
didn't make that change before committing, but I think it'd be a very
good idea.

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

Attachment

relcache_fix_warning.patch

Re: logical changeset generation v6.8

From

Andres Freund

Date:

13 December 2013, 14:12:37

On 2013-12-13 20:58:24 +1300, David Rowley wrote:
> On Wed, Dec 11, 2013 at 1:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> This introduced a new compiler warning on the visual studios build:
>   d:\postgres\b\src\backend\utils\cache\relcache.c(3958): warning C4715:
> 'RelationGetIndexAttrBitmap' : not all control paths return a value
> [D:\Postgres\b\postgres.vcxproj]
> 
> The attached patch fixes it.

I thought we'd managed to get elog(ERROR) properly annotated as noreturn
on msvc as well?

Greetings,

Andres Freund

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

Re: logical changeset generation v6.8

From

David Rowley

Date:

13 December 2013, 16:33:54

On Sat, Dec 14, 2013 at 12:12 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2013-12-13 20:58:24 +1300, David Rowley wrote:
> On Wed, Dec 11, 2013 at 1:11 PM, Robert Haas <robertmhaas@gmail.com> wrote:
> This introduced a new compiler warning on the visual studios build:
> d:\postgres\b\src\backend\utils\cache\relcache.c(3958): warning C4715:
> 'RelationGetIndexAttrBitmap' : not all control paths return a value
> [D:\Postgres\b\postgres.vcxproj]
>
> The attached patch fixes it.

I thought we'd managed to get elog(ERROR) properly annotated as noreturn
on msvc as well?

It looks like this is down to the elog macro, where the elevel is being assigned to a variable elevel_ then we're only doing pg_unreachable(); if elevel_ >= ERROR. The compiler must not be confident enough to optimise out the if condition even though the elevel is not changed after it is set from the constant.

Regards

David Rowley

Greetings,

Andres Freund

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

Re: logical changeset generation v6.8

From

Robert Haas

Date:

16 December 2013, 08:53:15

On Wed, Dec 11, 2013 at 7:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
>> I don't think there's much point in including "remapping" in all of
>> the error messages.  It adds burden for translators and users won't
>> know what a remapping file is anyway.
>
> It helps in locating wich part of the code caused a problem. I utterly
> hate to get reports with error messages that I can't correlate to a
> sourcefile. Yes, I know that verbose error output exists, but usually
> you don't get it that way... That said, I'll try to make the messages
> simpler.

Well, we could adopt a policy of not making messages originating from
different locations in the code the same.  However, it looks to me
like that's NOT the current policy, because some care has been taken
to reuse messages rather than distinguish them.  There's no hard and
fast rule here, because some cases are distinguished, but my gut
feeling is that all of the errors your patch introduces are
sufficiently obscure cases that separate messages with separate
translations are not warranted.  The time when this is likely to fail
is when someone borks the permissions on the data directory, and the
user probably won't need to care exactly which file we can't write.

>> +       if (!TransactionIdIsNormal(xmax))
>> +       {
>> +               /*
>> +                * no xmax is set, can't have any permanent ones, so
>> this check is
>> +                * sufficient
>> +                */
>> +       }
>> +       else if (HEAP_XMAX_IS_LOCKED_ONLY(new_tuple->t_data->t_infomask))
>> +       {
>> +               /* only locked, we don't care */
>> +       }
>> +       else if (!TransactionIdPrecedes(xmax, cutoff))
>> +       {
>> +               /* tuple has been deleted recently, log */
>> +               do_log_xmax = true;
>> +       }
>>
>> Obfuscated.  Rewrite without empty blocks.
>
> I don't understand why having an empty block is less clear than
> including a condition in several branches? Especially if the individual
> conditions might need to be documented?

It's not a coding style we typically use, to my knowledge.  Of course,
what is actually clearest is a matter of opinion, but my own
experience is that such blocks typically end up seeming clearer to me
when the individual comments are joined together into a paragraph that
explains the logic in full sentences what's going on.

>> +       /*
>> +        * Write out buffer everytime we've too many in-memory entries.
>> +        */
>> +       if (state->rs_num_rewrite_mappings >= 1000 /* arbitrary number */)
>> +               logical_heap_rewrite_flush_mappings(state);
>>
>> What happens if the number of items per hash table entry is small but
>> the number of entries is large?
>
> rs_num_rewrite_mappings is the overall number of in-memory mappings, not
> the number of per-entry mappings. That means we flush, even if all
> entries have only a couple of mappings, as soon as 1000 in memory
> entries have been collected. A bit simplistic, but seems okay enough?

Possibly, not sure yet.  I need to examine that logic in more detail,
but I had trouble following it and was hoping the next version would
be better-commented.

>> I'm still unhappy that we're introducing logical decoding slots but no
>> analogue for physical replication.  If we had the latter, would it
>> share meaningful amounts of structure with this?
>
> Yes, I think we could mostly reuse it, we'd probably want to add a field
> or two more (application_name, sync_prio?). I have been wondering
> whether some of the code in replication/logical/logical.c shouldn't be
> in replication/slot.c or similar. So far I've opted for leaving it in
> its current place since it would have to change a bit for a more general
> role.

I strongly favor moving the slot-related code to someplace with "slot"
in the name, and replication/slot.c seems about right.  Even if we
don't extend them to cover non-logical replication in this release,
we'll probably do it eventually, and it'd be better if that didn't
require moving large amounts of code between files.

> I still think that the technical parts of properly supporting persistent
> slots for physical rep aren't that hard, but that the behavioural
> decisions are. I think there are primarily two things for SR that we
> want to prevent using slots:
> a) removal of still used WAL (i.e. maintain knowledge about a standby's
>    last required REDO location)

Check.

> b) make hot_standby_feedback work across disconnections of the walsender
>    connection (i.e peg xmin, not just for catalogs though)

Check; might need to be optional.

> c) Make sure we can transport those across cascading
>    replication.

Not sure I follow.

> once those are there it's also useful to keep a bit more information
> about the state of replicas:
> * write, flush, apply lsn

Check.

> The hard questions that I see are like:
> * How do we manage standby registration? Does the admin have to do that
>   manually? Or does a standby register itself automatically if some config
>   paramter is set?
> * If automatically, how do we deal with the situation that registrant
>   dies before noting his own identifier somewhere persistent? My best idea
>   is a two phase registration process where registration in phase 1 are
>   thrown away after a restart, but yuck.

If you don't know the answers to these questions for the kind of
replication that we have now, then how do you know the answers for
logical replication?  Conversely, what makes the answers that you've
selected for logical replication unsuitable for our existing
replication?

I have to admit that before I saw your design for the logical
replication slots, the problem of making this work for our existing
replication stuff seemed almost intractable to me; I had no idea how
that was going to work. GUCs didn't seem suitable because I thought we
might need some data that is tabular in nature - i.e. configuration
specific to each slot.  And GUC has problems with that sort of thing.
And a new system catalog didn't seem good either, because you are
highly likely to want different configuration on the standby vs. on
the master.  But after I saw your design for the logical slots I said,
dude, get me some of that.  Keeping the data in shared memory,
persisting them across shutdowns, and managing them via either
function calls or the replication command language seems perfect.

Now, in terms of how registration works, whether for physical
replication or logical, it seems to me that the DBA will have to be
responsible for telling each client the name of the slot to which that
client should connect (omitting it at said DBA's option if, as for
physical replication, the slot is not mandatory).  Assuming such a
design, clients could elect for one of two possible behaviors when the
anticipated slot doesn't exist: either they error out, or they create
it.  Either is reasonable; we could even support both.  A third
alternative is to make each client responsible for generating a name,
but I think that's probably not as good.  If we went that route, the
name would presumably be some kind of random string, which will
probably be a lot less usable than a DBA-assigned name.  The client
would first generate it, second save it somewhere persistent (like a
file in the data directory), and third create a slot by that name.  If
the client crashes before creating the slot, it will find the name in
its persistent store after restart and, upon discovering that no such
slot exists, try again to create it.

But note that no matter which of those three options we pick, the
server support really need not work any differently.  I can imagine
any of them being useful and I don't care all that much which one we
end up with.  My personal preference is probably for manual
registration: if the DBA wants to use slots, said DBA will need to set
them up.  This also mitigates the issue you raise in your next point:
what is manually created will naturally also need to be manually
destroyed.

> * How do we deal with the usability wart that people *will* forget to
>   delete a slot when removing a node?

Aside from the point mentioned in the previous paragraph, we don't.
The fact that a standby which gets too far behind can't recover is a
usability wart, too.  I think this is a bit like asking what happens
if a user keeps inserting data of only ephemeral value into a table
and never deletes any of it as they ought to have done.  Well, they'll
be wasting disk space; potentially, they will fill the disk.  Sucks to
be them.  The solution is not to prohibit inserting data.

> * What requirements do we have for transporting knowlede about this
>   across a failover?
>
> I have little hope that we can resolve all that for 9.4.

I wouldn't consider this a requirement for a useful feature, and if it
is a requirement, then how are you solving it for logical replication?For physical replication, there's basically only
oneevent that needs

to be considered: master dead, promote standby.  That scenario can
also happen in logical replication... but there's a whole host of
other things that can happen, too.  An individual replication solution
may be single-writer but may, like Slony, allow that write authority
to be moved around.  Or it may have multiple writers.

Suppose A, B, and C replicate between themselves using mult-master
replication for write availability across geographies.  Within each
geo, A physically replicates to A', B to B', and C to C'.   There is
also a reporting server D which replicates selected tables from each
of A, B, and C.  On a particularly bad day, D is switched to replicate
a particular table from B rather than A while at the same time B is
failed over to B' in the midst of a network outage that renders B and
B' only intermittently network-accessible, leading to a high rate of
multi-master conflicts with C that require manual resolution, which
doesn't happen until the following week.  In the meantime, B' is
failed back to B and C is temporarily removed from the multi-master
cluster and allowed to run standalone.  If you can develop a
replication solution that leaves D with the correct data at the end of
all of that, my hat is off to you ... and the problem of getting all
this to work when only physical replication is in use and then number
of possible scenarios is much less ought to seem simple by comparison.

But whether it does or not, I don't see why we have to solve it in
this release.  Following standby promotions, etc. is a whole feature,
or several, unto itself.

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

Re: logical changeset generation v6.8

From

Andres Freund

Date:

16 December 2013, 14:01:44

Hi Robert,

On 2013-12-16 00:53:10 -0500, Robert Haas wrote:
> On Wed, Dec 11, 2013 at 7:08 PM, Andres Freund <andres@2ndquadrant.com> wrote:
> >> I don't think there's much point in including "remapping" in all of
> >> the error messages.  It adds burden for translators and users won't
> >> know what a remapping file is anyway.
> >
> > It helps in locating wich part of the code caused a problem. I utterly
> > hate to get reports with error messages that I can't correlate to a
> > sourcefile. Yes, I know that verbose error output exists, but usually
> > you don't get it that way... That said, I'll try to make the messages
> > simpler.
> 
> Well, we could adopt a policy of not making messages originating from
> different locations in the code the same.  However, it looks to me
> like that's NOT the current policy, because some care has been taken
> to reuse messages rather than distinguish them.

To me that mostly looks like cases where we either don't want to tell
more for various security-ish purposes or where they have been copy and
pasted...

> There's no hard and
> fast rule here, because some cases are distinguished, but my gut
> feeling is that all of the errors your patch introduces are
> sufficiently obscure cases that separate messages with separate
> translations are not warranted.

Perhaps we should just introduce a marker that some such strings are not
to be translated if they are of the unexpected kind. That would probably
make debugging easier too ;)

> I need to examine that logic in more detail,
> but I had trouble following it and was hoping the next version would
> be better-commented.

Yea, I've started expanding the comments about the higher level concerns
- I've been so knee deep in this that I didn't realize they weren't
there.

> > b) make hot_standby_feedback work across disconnections of the walsender
> >    connection (i.e peg xmin, not just for catalogs though)
> 
> Check; might need to be optional.

Yea, I am pretty sure it will. It'd probably pretty nasty to set
min_recovery_apply_delay=7d and force xmin kept to that...

> > c) Make sure we can transport those across cascading
> >    replication.
> 
> Not sure I follow.

Consider a replication scenario like primary <-> standby-1 <->
standby-2. The primary may not only not remove data that standby-1
requires, but also not data that standby-2 needs. Not really necessary
for WAL since that will also reside on standby-1 but definitely for the
xmin horizon.
So standby-1 will need to signal not only his own needs, but also of the
nodes below.

> > The hard questions that I see are like:
> > * How do we manage standby registration? Does the admin have to do that
> >   manually? Or does a standby register itself automatically if some config
> >   paramter is set?
> > * If automatically, how do we deal with the situation that registrant
> >   dies before noting his own identifier somewhere persistent? My best idea
> >   is a two phase registration process where registration in phase 1 are
> >   thrown away after a restart, but yuck.
>
> If you don't know the answers to these questions for the kind of
> replication that we have now, then how do you know the answers for
> logical replication?  Conversely, what makes the answers that you've
> selected for logical replication unsuitable for our existing
> replication?

There's a pretty fundamental difference imo - with the logical decoding
stuff we only supply support for change producing nodes, with physical
rep we supply both.
There's no need to decide about the way node ids are stored in in-core logical
rep. consumers since there are no in-core ones. Yet. Also, physical rep
by now is a pretty established thing, we need to be much more careful
about compatibility there.

> I have to admit that before I saw your design for the logical
> replication slots, the problem of making this work for our existing
> replication stuff seemed almost intractable to me; I had no idea how
> that was going to work.

Believe me, it caused me some headaches to deceive it for decoding
too. Oh, and I think I watched just about all episodes of some stupid TV
show during it ;)

> Keeping the data in shared memory,
> persisting them across shutdowns, and managing them via either
> function calls or the replication command language seems perfect.

Thanks. I think the concept has quite some merits. The implementation is
a bit simplistic atm, we e.g. might want to work harder at coalescing
fsync()s and such, but that's a further step when we see whether it's
worthwile in the real world.

> Now, in terms of how registration works, whether for physical
> replication or logical, it seems to me that the DBA will have to be
> responsible for telling each client the name of the slot to which that
> client should connect (omitting it at said DBA's option if, as for
> physical replication, the slot is not mandatory).

It seems reasonable to me to reuse the application_name for the slot's
name, similar to the way it's used for synchronous rep. It seems odd to
use two different ways to identify nodes. t should probably only be part
of final slot name though, with the rest being autogenerated.

> Assuming such a design, clients could elect for one of two possible
> behaviors when the anticipated slot doesn't exist: either they error
> out, or they create it.  Either is reasonable; we could even support
> both.

For physical rep, I don't see too much argument for not autogenerating
it. The one I can see it that it makes accidentally changing a slots
name easier, with the consequence of leaving a unused slot around.

>  Athird
> alternative is to make each client responsible for generating a name,
> but I think that's probably not as good.  If we went that route, the
> name would presumably be some kind of random string, which will
> probably be a lot less usable than a DBA-assigned name.  The client
> would first generate it, second save it somewhere persistent (like a
> file in the data directory), and third create a slot by that name.  If
> the client crashes before creating the slot, it will find the name in
> its persistent store after restart and, upon discovering that no such
> slot exists, try again to create it.

I thought we'd need to go that route for a while, but I struggled
exactly with the kind of races you desribe. I'd already toyed with ideas
of making slots "ephemeral" intially, until they get confirmed by the
standby. Essentially reinventing 2PC...
Not needing to solve those problems sounds like a good idea.

> But note that no matter which of those three options we pick, the
> server support really need not work any differently.

Yea, that part isn't worrying me overly much - there's really not much
beside passing the slot name before/in START_REPLICATION that needs to
be done.

> > * How do we deal with the usability wart that people *will* forget to
> >   delete a slot when removing a node?
> 
> Aside from the point mentioned in the previous paragraph, we don't.
> The fact that a standby which gets too far behind can't recover is a
> usability wart, too.  I think this is a bit like asking what happens
> if a user keeps inserting data of only ephemeral value into a table
> and never deletes any of it as they ought to have done.  Well, they'll
> be wasting disk space; potentially, they will fill the disk.  Sucks to
> be them.  The solution is not to prohibit inserting data.

I am perfectly happy with taking that stance - in previous discussions
some (most notably Peter G.) argued ardently against it though.

I think we need to improve the monitoring facilities a bit, and that
should be it. Like
* expose xmin in pg_stat_activity, pg_prepared_xacts, pg_replication_slots (or whatever it's going to be called)
* expose the last restartpoint's redo pointer in pg_stat_replication, pg_replication_slots

That said, the consequences can be a bit harsher than a full disk - the
anti-wraparound security measures might kick in requiring a restart into
single user mode. That's way more confusing than cleaning up a bit of
space on the disk.

> > * What requirements do we have for transporting knowlede about this
> >   across a failover?

> I wouldn't consider this a requirement for a useful feature, and if it
> is a requirement, then how are you solving it for logical replication?

Failover for physical rep and for logical rep are pretty different
beasts imo - I don't think they can easily be compared. Especially since
we won't deliver a full logical rep solution and as you say it will look
pretty different depending on the individual solution.

Consider what happens though, if you promote a node for physical rep. As
soon as it's promoted, it will accept writes and then start a
checkpoint. Unless other standbys have started to follow that node
before either that checkpoint happens (removing WAL) or
autovacuuming/hot-pruning is performed (creating recovery conflicts),
we'll possibly loose the data required to let the standbys follow the
promotion. Note that wal_keep_segments and vacuum_defer_cleanup_age both
sorta work for that...

Could somebody please deliver me a time dilation device?

Regards,

Andres Freund

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

Re: logical changeset generation v6.8

From

Robert Haas

Date:

16 December 2013, 20:07:15

On Mon, Dec 16, 2013 at 6:01 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> There's no hard and
>> fast rule here, because some cases are distinguished, but my gut
>> feeling is that all of the errors your patch introduces are
>> sufficiently obscure cases that separate messages with separate
>> translations are not warranted.
>
> Perhaps we should just introduce a marker that some such strings are not
> to be translated if they are of the unexpected kind. That would probably
> make debugging easier too ;)

Well, we have that: it's called elog.  But that doesn't seem like the
right thing here.

>> > b) make hot_standby_feedback work across disconnections of the walsender
>> >    connection (i.e peg xmin, not just for catalogs though)
>>
>> Check; might need to be optional.
>
> Yea, I am pretty sure it will. It'd probably pretty nasty to set
> min_recovery_apply_delay=7d and force xmin kept to that...

Yes, that would be... unfortunate.

>> > c) Make sure we can transport those across cascading
>> >    replication.
>>
>> Not sure I follow.
>
> Consider a replication scenario like primary <-> standby-1 <->
> standby-2. The primary may not only not remove data that standby-1
> requires, but also not data that standby-2 needs. Not really necessary
> for WAL since that will also reside on standby-1 but definitely for the
> xmin horizon.
> So standby-1 will need to signal not only his own needs, but also of the
> nodes below.

True.

>> > The hard questions that I see are like:
>> > * How do we manage standby registration? Does the admin have to do that
>> >   manually? Or does a standby register itself automatically if some config
>> >   paramter is set?
>> > * If automatically, how do we deal with the situation that registrant
>> >   dies before noting his own identifier somewhere persistent? My best idea
>> >   is a two phase registration process where registration in phase 1 are
>> >   thrown away after a restart, but yuck.
>>
>> If you don't know the answers to these questions for the kind of
>> replication that we have now, then how do you know the answers for
>> logical replication?  Conversely, what makes the answers that you've
>> selected for logical replication unsuitable for our existing
>> replication?
>
> There's a pretty fundamental difference imo - with the logical decoding
> stuff we only supply support for change producing nodes, with physical
> rep we supply both.

I'm not sure I follow this.  "Both" what and what?

> There's no need to decide about the way node ids are stored in in-core logical
> rep. consumers since there are no in-core ones. Yet.

I don't know that we have or need to make any judgements about how to
store node IDs.  You have decided that slots have names, and I see no
problem there.

> Also, physical rep
> by now is a pretty established thing, we need to be much more careful
> about compatibility there.

I don't think we should change anything in backward-incompatible
fashion.  If we add any new behavior, it'd surely be optional.

> I think we need to improve the monitoring facilities a bit, and that
> should be it. Like
> * expose xmin in pg_stat_activity, pg_prepared_xacts,
>   pg_replication_slots (or whatever it's going to be called)
> * expose the last restartpoint's redo pointer in pg_stat_replication, pg_replication_slots

+1.

> That said, the consequences can be a bit harsher than a full disk - the
> anti-wraparound security measures might kick in requiring a restart into
> single user mode. That's way more confusing than cleaning up a bit of
> space on the disk.

Yes, true.  I'm not sure what approach to that problem is best.  It's
long seemed to me that well before we get to the point of shutting
down the whole cluster we ought to just start killing sessions with
old xmins.  But that doesn't generalize well to prepared transactions,
which can't just be rolled back or committed without guidance; and
killing slots seems a bit dicey too.

> Consider what happens though, if you promote a node for physical rep. As
> soon as it's promoted, it will accept writes and then start a
> checkpoint. Unless other standbys have started to follow that node
> before either that checkpoint happens (removing WAL) or
> autovacuuming/hot-pruning is performed (creating recovery conflicts),
> we'll possibly loose the data required to let the standbys follow the
> promotion. Note that wal_keep_segments and vacuum_defer_cleanup_age both
> sorta work for that...

True.

> Could somebody please deliver me a time dilation device?

Upon reflection, I am less concerned with actually having physical
slots in this release than I am with making sure we're not boxing
ourselves into a corner that will make them hard to add later.  If
we've got a clear design that can be generalized to that case, but the
SMOP required exceeds what can be done in the time available, I am OK
to punt it.  But I am not sure we're at that point yet.

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

Re: logical changeset generation v6.8

From

Tom Lane

Date:

16 December 2013, 20:21:53

Robert Haas <robertmhaas@gmail.com> writes:
> On Mon, Dec 16, 2013 at 6:01 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>> Perhaps we should just introduce a marker that some such strings are not
>> to be translated if they are of the unexpected kind. That would probably
>> make debugging easier too ;)

> Well, we have that: it's called elog.  But that doesn't seem like the
> right thing here.

errmsg_internal?
        regards, tom lane

Re: logical changeset generation v6.8

From

Robert Haas

Date:

16 December 2013, 20:48:12

On Mon, Dec 16, 2013 at 12:21 PM, Tom Lane <tgl@sss.pgh.pa.us> wrote:
> Robert Haas <robertmhaas@gmail.com> writes:
>> On Mon, Dec 16, 2013 at 6:01 AM, Andres Freund <andres@2ndquadrant.com> wrote:
>>> Perhaps we should just introduce a marker that some such strings are not
>>> to be translated if they are of the unexpected kind. That would probably
>>> make debugging easier too ;)
>
>> Well, we have that: it's called elog.  But that doesn't seem like the
>> right thing here.
>
> errmsg_internal?

There's that, too.  But again, these messages are not can't-happen
scenarios.  The argument is just whether to reuse existing error
message text (like "could not write file") or invent a new variation
(like "could not write remapping file").  Andres' argument (which is
valid) is that distinguished messages make it easier to troubleshoot
without needing to turn on verbose error messages.  My argument (which
I think is also valid) is that a user isn't likely to know what a
remapping file is, and having more messages increases the translation
burden.  Is there a project policy on this topic?

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

Re: logical changeset generation v6.8

From

Alvaro Herrera

Date:

16 December 2013, 21:52:06

Robert Haas escribió:

> There's that, too.  But again, these messages are not can't-happen
> scenarios.  The argument is just whether to reuse existing error
> message text (like "could not write file") or invent a new variation
> (like "could not write remapping file").  Andres' argument (which is
> valid) is that distinguished messages make it easier to troubleshoot
> without needing to turn on verbose error messages.  My argument (which
> I think is also valid) is that a user isn't likely to know what a
> remapping file is, and having more messages increases the translation
> burden.  Is there a project policy on this topic?

I would vote for a generic "could not write file %s" where the %s lets
the troubleshooter know the path of the file, and thus in what context
it is being read.  We already have a similar case where slru.c reports
error as pertaining to "transaction 12345" but the path is
"pg_subtrans/xyz" or multixact etc; while it doesn't explicitely say
what module is raising the error, it's pretty clear from the path.

Would that not work here?

-- 
Álvaro Herrera                http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Training & Services

Re: logical changeset generation v6.8

From

Tom Lane

Date:

16 December 2013, 22:14:05

Robert Haas <robertmhaas@gmail.com> writes:
> There's that, too.  But again, these messages are not can't-happen
> scenarios.  The argument is just whether to reuse existing error
> message text (like "could not write file") or invent a new variation
> (like "could not write remapping file").

As long as the message includes the file name, which it surely oughta,
I don't see that we need any explanation of what Postgres thinks the
file is for.  If someone cares about that they can reverse-engineer
it from the file name; while as you said upthread, most of the time
the directory path is going to be the key piece of useful information.

So +1 for "could not write file".

> Andres' argument (which is
> valid) is that distinguished messages make it easier to troubleshoot
> without needing to turn on verbose error messages.  My argument (which
> I think is also valid) is that a user isn't likely to know what a
> remapping file is, and having more messages increases the translation
> burden.  Is there a project policy on this topic?

I think Andres' argument is a thinly veiled version of "let's put the
routine name into the message text", which there definitely is project
policy against (see 49.3.13 in the message style guide).  If you want to
know the code location where the error was thrown, the answer is to get
a verbose log, not to put identifying information into the user-facing
message text.  And this is only partially-identifying information,
which seems like the worst of both worlds: you've got confused users and
overworked translators, and you still don't know exactly where it was
thrown from.
        regards, tom lane

Re: logical changeset generation v6.8

From

Andres Freund

Date:

17 December 2013, 15:48:16

On 2013-12-16 00:53:10 -0500, Robert Haas wrote:
> > Yes, I think we could mostly reuse it, we'd probably want to add a field
> > or two more (application_name, sync_prio?). I have been wondering
> > whether some of the code in replication/logical/logical.c shouldn't be
> > in replication/slot.c or similar. So far I've opted for leaving it in
> > its current place since it would have to change a bit for a more general
> > role.
> 
> I strongly favor moving the slot-related code to someplace with "slot"
> in the name, and replication/slot.c seems about right.  Even if we
> don't extend them to cover non-logical replication in this release,
> we'll probably do it eventually, and it'd be better if that didn't
> require moving large amounts of code between files.

Any opinion on the storage location of the slot files? It's currently
pg_llog/$slotname/state[.tmp]. It's a directory so we have a location
during logical decoding to spill data to...

Greetings,

Andres Freund

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

Re: logical changeset generation v6.8

From

Robert Haas

Date:

17 December 2013, 17:32:22

On Tue, Dec 17, 2013 at 7:48 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> On 2013-12-16 00:53:10 -0500, Robert Haas wrote:
>> > Yes, I think we could mostly reuse it, we'd probably want to add a field
>> > or two more (application_name, sync_prio?). I have been wondering
>> > whether some of the code in replication/logical/logical.c shouldn't be
>> > in replication/slot.c or similar. So far I've opted for leaving it in
>> > its current place since it would have to change a bit for a more general
>> > role.
>>
>> I strongly favor moving the slot-related code to someplace with "slot"
>> in the name, and replication/slot.c seems about right.  Even if we
>> don't extend them to cover non-logical replication in this release,
>> we'll probably do it eventually, and it'd be better if that didn't
>> require moving large amounts of code between files.
>
> Any opinion on the storage location of the slot files? It's currently
> pg_llog/$slotname/state[.tmp]. It's a directory so we have a location
> during logical decoding to spill data to...

pg_replslot?  pg_replication_slot?

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

Re: logical changeset generation v6

From

Magnus Hagander

Date:

24 April 2014, 10:39:27

On Mon, Sep 23, 2013 at 7:03 PM, Peter Geoghegan <pg@heroku.com> wrote:

On Mon, Sep 23, 2013 at 9:54 AM, Andres Freund <andres@2ndquadrant.com> wrote:
> I still find it wierd/inconsistent to have:
> * pg_receivexlog
> * pg_recvlogical
> binaries, even from the same source directory. Why once "pg_recv" and
> once "pg_receive"?

+1

Digging up a really old thread since I just got annoyed by the inconsistent naming the first time myself :)

I can't find that this discussion actually came to a proper consensus, but I may be missing something. Did we go with pg_recvlogical just because we couldn't decide on a better name, or did we intentionally decide it was the best?

I definitely think pg_receivelogical would be a better name, for consistency (because it's way too late to rename pg_receivexlog of course - once released that can't really chance. Which is why *if* we want to change the name of pg_recvxlog we have a few more days to make a decision..)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Re: logical changeset generation v6

From

Andres Freund

Date:

24 April 2014, 10:44:00

On 2014-04-24 09:39:21 +0200, Magnus Hagander wrote:
> I can't find that this discussion actually came to a proper consensus, but
> I may be missing something. Did we go with pg_recvlogical just because we
> couldn't decide on a better name, or did we intentionally decide it was the
> best?

I went with pg_recvlogical because that's where the (small) majority
seemed to be. Even if I was unconvinced. There were so many outstanding
big fights at that point that I didn't want to spend my time on this ;)

Greetings,

Andres Freund

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

Re: logical changeset generation v6

From

Magnus Hagander

Date:

24 April 2014, 10:46:12

On Thu, Apr 24, 2014 at 9:43 AM, Andres Freund <andres@2ndquadrant.com> wrote:

On 2014-04-24 09:39:21 +0200, Magnus Hagander wrote:
> I can't find that this discussion actually came to a proper consensus, but
> I may be missing something. Did we go with pg_recvlogical just because we
> couldn't decide on a better name, or did we intentionally decide it was the
> best?

I went with pg_recvlogical because that's where the (small) majority
seemed to be. Even if I was unconvinced. There were so many outstanding
big fights at that point that I didn't want to spend my time on this ;)

I was guessing something like the second part there, which is why I figured this would be a good time to bring this fight back up to the surface ;)

--
Magnus Hagander
Me: http://www.hagander.net/
Work: http://www.redpill-linpro.com/

Re: logical changeset generation v6

From

Andres Freund

Date:

24 April 2014, 10:50:49

On 2014-04-24 09:46:07 +0200, Magnus Hagander wrote:
> On Thu, Apr 24, 2014 at 9:43 AM, Andres Freund <andres@2ndquadrant.com>wrote:
> 
> > On 2014-04-24 09:39:21 +0200, Magnus Hagander wrote:
> > > I can't find that this discussion actually came to a proper consensus,
> > but
> > > I may be missing something. Did we go with pg_recvlogical just because we
> > > couldn't decide on a better name, or did we intentionally decide it was
> > the
> > > best?
> >
> > I went with pg_recvlogical because that's where the (small) majority
> > seemed to be. Even if I was unconvinced. There were so many outstanding
> > big fights at that point that I didn't want to spend my time on this ;)
> >
> >
> I was guessing something like the second part there, which is why I figured
> this would be a good time to bring this fight back up to the surface ;)

I have to admit that I still don't care too much. By now I'd tentatively
want to stay with the current name because that's what I got used to,
but if somebody else has strong opinions and finds concensus...

Greetings,

Andres Freund

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