Re: logical changeset generation v6

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.

Greetings,

Andres Freund

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

What's with 0001-Improve-regression-test-for-8410.patch? Did you mean
to include that?

On 2013-09-15 10:03:54 -0400, Peter Eisentraut wrote:
> What's with 0001-Improve-regression-test-for-8410.patch? Did you mean
> to include that?

Gah, no. That's already committed and unrelated. Stupid wildcard.

Greetings,

Andres Freund

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

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.

(I guess that's really pgcompinclude's job to find out, but
cpluspluscheck seems to be easier to use.)

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.

Thanks,

Andres Freund

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

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

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.

Greetings,

Andres Freund

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

On Tue, Sep 17, 2013 at 11:31 PM, Andres Freund <andres(at)2ndquadrant(dot)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

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 Tue, Sep 17, 2013 at 10:31 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Rebased patches attached.

I spent a bit of time looking at these patches yesterday and today.
It seems to me that there's a fair amount of stylistic cleanup that is
still needed, and some pretty bad naming choices, and some FIXMEs that
should probably be fixed, but for an initial review email it seems
more helpful to focus on high-level design, so here goes.

- Looking specifically at the 0004 patch, I think that the
RecentGlobalDataXmin changes are logically separate from the rest of
the patch, and that if we're going to commit them at all, it should be
separate from the rest of this. I think this is basically a
performance optimization. AFAICS, there's no correctness problem with
the idea of continuing to maintain a single RecentGlobalXmin; it's
just that you'll potentially end up with quite a lot of bloat. But
that's not an argument that those two things HAVE to be committed
together; either could be done first, and independently of the other.
Also, these changes are quite complex and it's different to form a
judgement as to whether this idea is safe when they are intermingled
with the rest of the logical replication stuff.

More generally, the thing that bugs me about this approach is that
logical replication is not really special, but what you've done here
MAKES it special. There are plenty of other situations where we are
too aggressive about holding back xmin. A single-statement
transaction that selects from a single table holds back xmin for the
entire cluster, and that is Very Bad. It would probably be unfair to
say, well, you have to solve that problem first. But on the other
hand, how do we know that the approach you've adopted here isn't going
to make the more general problem harder to solve? It seems invasive
at a pretty low level. I think we should at least spend some time
thinking about what *general* solutions to this problem would like
like and then decide whether this is approach is likely to be
forward-compatible with those solutions.

- There are no changes to the "doc" directory. Obviously, if you're
going to add a new value for the wal_level GUC, it's gonna need to be
documented. Similarly, pg_receivellog needs to be documented. In all
likelihood, you also need a whole chapter providing general background
on this technology. A couple of README files is not going to do it,
and those files aren't suitable for check-in anyway (e.g. DESIGN.txt
makes reference to a URL where the current version of some patch can
be found; that's not appropriate for permanent documentation). But
aside from that, what we really need here is user documentation, not
developer documentation. I can perhaps pass judgement on whether the
guts of this functionality do things that are fundamentally unsafe,
but whether the user interface is good or bad is a question that
deserves broader input, and without documentation, most people aren't
going to understand it well enough to know whether they like it. And
TBH, *I* don't really want to reverse-engineer what pg_receivellog
does from a read-through of the code, either.

- Given that we don't reassemble transactions until commit time, why
do we need to to ensure that XIDs are logged before their sub-XIDs
appear in WAL? As I've said previously, I actually think that
on-the-fly reassembly is probably going to turn out to be very
important. But if we're not getting that, do we really need this?
Also, while I'm trying to keep this email focused on high-level
concerns, I have to say that guaranteedlyLogged has got to be one of
the worst variable names I've ever seen, starting (but not ending)
with the fact that guaranteedly is not a word. I'm also tempted to
say that all of the wal_level=logical changes should be split out as
their own patch, separate from the decoding stuff. Personally, I
would find that much easier to review, although I admit it's less
critical here than for the RecentGlobalDataXmin stuff.

- If XLOG_HEAP_INPLACE is not decoded, doesn't that mean that this
facility can't be used to replicate a system catalog table? Is that a
restriction we should enforce/document somehow?

- The new code is rather light on comments. decode.c is extremely
light. For example, consider the function DecodeAbort(), which
contains the following comment:

+ /*
+ * this is a bit grotty, but if we're "faking" an abort we've
already gone
+ * through
+ */

Well, I have no idea what that means. I'm sure you do, but I bet the
next person who isn't you that tries to understand this probably
won't. It's also probably true that I could figure it out if I spent
more time on it, but I think the point of comments is to keep the
amount of time that must be spent trying to understand code to a
manageable level. Generally, I'd suggest that any non-trivial
functions in these files should have a header comment explaining what
their job is; e.g. for DecodeStandbyOp you could write something like
"Decode an RM_STANDBY WAL record. Currently, we only care about
XLOG_RUNNING_XACTS records, which tell us about transactions that may
have aborted when without writing an explicit abort record." Or
whatever the right explanation is. And then particularly tricky bits
should have their own comments.

- It still bothers me that we're going to have mandatory slots for
logical replication and no slots for physical replication. Why are
slots mandatory in one case and not even allowable in the other? Is
it sensible to think about slotless logical replication - or maybe I
should say, is it any LESS sensible than slotless physical
replication?

- What is the purpose of (Un)SuspendDecodingSnapshots? It seems that
should be explained somewhere. I have my doubts about how safe that
is. And I definitely think that SetupDecodingSnapshots() is not OK.
Overwriting the satisfies functions in static pointers may be a great
way to make sure you've covered all bases during development, but I
can't see us wanting that ugliness in the official sources.

- I don't really like "time travel" as a name for reconstructing a
previous snapshot of a catalog. Maybe it's as good as anything, but
it also doesn't help that "during decoding" is used in some places to
refer to the same concept. I wonder if we should call these "logical
replication snapshots" or "historical MVCC snapshots" or somesuch and
then try to make the terminology consistent throughout.
ReorderBufferTXN->does_timetravel really means "time travel will be
needed to decode what this transaction did", which is not really the
same thing.

That's as much relatively-big-picture stuff as I'm able to notice on a
first read-through.

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

Hi Robert,

On 2013-09-19 10:02:31 -0400, Robert Haas wrote:
> On Tue, Sep 17, 2013 at 10:31 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Rebased patches attached.
>
> I spent a bit of time looking at these patches yesterday and today.
> It seems to me that there's a fair amount of stylistic cleanup that is
> still needed, and some pretty bad naming choices, and some FIXMEs that
> should probably be fixed, but for an initial review email it seems
> more helpful to focus on high-level design, so here goes.

Thanks for looking at it.

Yes, I think the highlevel stuff is the important bit.

As you note, the documentation needs to be written and that's certainly
not a small task. But doing so before the highlevel design is agreed
upon makes it too likely that it will need to be entirely scrapped.

> - Looking specifically at the 0004 patch, I think that the
> RecentGlobalDataXmin changes are logically separate from the rest of
> the patch, and that if we're going to commit them at all, it should be
> separate from the rest of this. I think this is basically a
> performance optimization. AFAICS, there's no correctness problem with
> the idea of continuing to maintain a single RecentGlobalXmin; it's
> just that you'll potentially end up with quite a lot of bloat. But
> that's not an argument that those two things HAVE to be committed
> together; either could be done first, and independently of the other.
> Also, these changes are quite complex and it's different to form a
> judgement as to whether this idea is safe when they are intermingled
> with the rest of the logical replication stuff.

Up until v3 the RecentGlobalDataXmin stuff wasn't included and reviewers
(primarily Peter G. on -hackers and Greg Stark at pgconf.eu) remarked on
that and considered it critical. I argued for a considerable amount of
time that it shouldn't be done in an initial patch and then gave in.

They have a point though, if you e.g. replicate a pgbench -j16 workload
the addition of RecentGlobalDataXmin reduces the performance impact of
replication from about 60% to less than 5% in my measurements. Turns out
heap pruning is damn critical for that kind of workload.

> More generally, the thing that bugs me about this approach is that
> logical replication is not really special, but what you've done here
> MAKES it special. There are plenty of other situations where we are
> too aggressive about holding back xmin. A single-statement
> transaction that selects from a single table holds back xmin for the
> entire cluster, and that is Very Bad. It would probably be unfair to
> say, well, you have to solve that problem first. But on the other
> hand, how do we know that the approach you've adopted here isn't going
> to make the more general problem harder to solve? It seems invasive
> at a pretty low level.

The reason why I think it's actually different is that the user actually
has control over how long transactions are running on the primary. They
don't really control how fast a replication consumer consumes and how
often it sends feedback messages.

> I think we should at least spend some time
> thinking about what *general* solutions to this problem would like
> like and then decide whether this is approach is likely to be
> forward-compatible with those solutions.

I thought about the general case for a good bit and decided that all
solutions that work in a more general scenario are complex enough that I
don't want to implement them. And I don't really see any backward
compatibility concerns here - removing the logic of using a separate
horizon for user tables in contrast to system tables is pretty trivial
and shouldn't have any external effect. Except pegging the horizon more,
but that's what the new approach would fix, right?

> - Given that we don't reassemble transactions until commit time, why
> do we need to to ensure that XIDs are logged before their sub-XIDs
> appear in WAL?

Currently it's important to know where the oldest transaction that is
alive started at to determine from where we need to restart
decoding. That's done by keeping a lsn-ordered list of in progress
toplevel transactions. The above invariant makes it cheap to maintain
that list.

> As I've said previously, I actually think that
> on-the-fly reassembly is probably going to turn out to be very
> important. But if we're not getting that, do we really need this?

It's also preparatory for supporting that.

I agree that it's pretty important, but after actually having
implemented a replication solution using this, I still think that most
usecase won't using it when available. I plan to work on implementing
that.

> Also, while I'm trying to keep this email focused on high-level
> concerns, I have to say that guaranteedlyLogged has got to be one of
> the worst variable names I've ever seen, starting (but not ending)
> with the fact that guaranteedly is not a word. I'm also tempted to
> say that all of the wal_level=logical changes should be split out as
> their own patch, separate from the decoding stuff. Personally, I
> would find that much easier to review, although I admit it's less
> critical here than for the RecentGlobalDataXmin stuff.

I can do that again and it actually was that way in the past. But
there's no user for it before the later patch and it's hard to
understand the reasoning for the changed wal logging separately, that's
why I merged it at some point.

> - If XLOG_HEAP_INPLACE is not decoded, doesn't that mean that this
> facility can't be used to replicate a system catalog table? Is that a
> restriction we should enforce/document somehow?

Currently catalog tables aren't replicated, yes. They simply are skipped
during decoding. XLOG_HEAP_INPLACE isn't the primary reason for that
though.

Do you see a usecase for it?.

> - The new code is rather light on comments. decode.c is extremely
> light.

Will improve. I think most of the other code is better commented, but it
still could use quite a bit of improvement nonethless.

> - It still bothers me that we're going to have mandatory slots for
> logical replication and no slots for physical replication. Why are
> slots mandatory in one case and not even allowable in the other? Is
> it sensible to think about slotless logical replication - or maybe I
> should say, is it any LESS sensible than slotless physical
> replication?

Well, as you know, I do want to have slots for physical replication as
well. But there actually is a fundamental difference why we need it for
logical rep and not for physical: In physical replication, if the xmin
progresses too far, client queries will be cancelled. Annoying but not
fatal. In logical replication we will not be able to continue
replicating since we cannot decode the WAL stream without a valid
catalog snapshot. If xmin already has progressed too far the tuples
won't be there anymore.

If people think this needs to be a general facility from the start, I
can be convinced that way, but I think there's so much to discuss around
the semantics and different usecases that I'd much prefer to discuss
that later.

> - What is the purpose of (Un)SuspendDecodingSnapshots? It seems that
> should be explained somewhere. I have my doubts about how safe that
> is.

I'll document the details if they aren't right now. Consider what
happens if somebody does something like: "VACUUM FULL pg_am;". If we
were to build the relation descriptor of pg_am in an "historical
snapshot", as you coin it, we'd have the wrong filenode in there. And
consequently any future lookups in pg_am will open a file that doesn't
exist.
That problem only exist for non-nailed relations that are accessed
during decoding.

> And I definitely think that SetupDecodingSnapshots() is not OK.
> Overwriting the satisfies functions in static pointers may be a great
> way to make sure you've covered all bases during development, but I
> can't see us wanting that ugliness in the official sources.

Yes, I don't like it either. I am not sure what to replace it with
though. It's easy enough to fit something in GetCatalogSnapshot() and I
don't have a problem with that, but I am less happy with adding code
like that to GetSnapshotData() for callers that use explicit snapshots.

> - I don't really like "time travel" as a name for reconstructing a
> previous snapshot of a catalog. Maybe it's as good as anything, but
> it also doesn't help that "during decoding" is used in some places to
> refer to the same concept.

Heh, I think that's me trying to avoid repeating the same term over and
over subconsciously.

> I wonder if we should call these "logical replication snapshots" or
> "historical MVCC snapshots" or somesuch and then try to make the
> terminology consistent throughout. ReorderBufferTXN->does_timetravel
> really means "time travel will be needed to decode what this
> transaction did", which is not really the same thing.

Hm. ->does_timetravel really is badly named. Yuck. Should be
'->modifies_catalog' or similar.

I'll think whether I can agree with either of the suggested terms or can
think of a better one. Till then I'll try to make the comments more
consistent.

Thanks!

Andres Freund

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

On Thu, Sep 19, 2013 at 10:43 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> - Looking specifically at the 0004 patch, I think that the
>> RecentGlobalDataXmin changes are logically separate from the rest of
>> the patch, and that if we're going to commit them at all, it should be
>> separate from the rest of this. I think this is basically a
>> performance optimization. AFAICS, there's no correctness problem with
>> the idea of continuing to maintain a single RecentGlobalXmin; it's
>> just that you'll potentially end up with quite a lot of bloat. But
>> that's not an argument that those two things HAVE to be committed
>> together; either could be done first, and independently of the other.
>> Also, these changes are quite complex and it's different to form a
>> judgement as to whether this idea is safe when they are intermingled
>> with the rest of the logical replication stuff.
>
> Up until v3 the RecentGlobalDataXmin stuff wasn't included and reviewers
> (primarily Peter G. on -hackers and Greg Stark at pgconf.eu) remarked on
> that and considered it critical. I argued for a considerable amount of
> time that it shouldn't be done in an initial patch and then gave in.
>
> They have a point though, if you e.g. replicate a pgbench -j16 workload
> the addition of RecentGlobalDataXmin reduces the performance impact of
> replication from about 60% to less than 5% in my measurements. Turns out
> heap pruning is damn critical for that kind of workload.

No question. I'm not saying that that optimization shouldn't go in
right after the main patch does, but IMHO right now there are too many
things going in the 0004 patch to discuss them all simultaneously.
I'd like to find a way of splitting this up that will let us
block-and-tackle individual pieces of it, even we end up committing
them all one right after the other.

But that raises an interesting question: why is the overhead so bad?
I mean, this shouldn't be any worse than having a series of
transactions running concurrently with pgbench that take a snapshot
and hold it for as long as it takes the decoding process to decode the
most-recently committed transaction. Is the issue here that we can't
advance xmin until we've fsync'd the fruits of decoding down to disk?
If so, that's mighty painful. But we'd really only need to hold back
xmin in that situation when some catalog change has occurred
meanwhile, which for pgbench means never. So something seems fishy
here.

> I thought about the general case for a good bit and decided that all
> solutions that work in a more general scenario are complex enough that I
> don't want to implement them. And I don't really see any backward
> compatibility concerns here - removing the logic of using a separate
> horizon for user tables in contrast to system tables is pretty trivial
> and shouldn't have any external effect. Except pegging the horizon more,
> but that's what the new approach would fix, right?

Hmm, maybe.

>> Also, while I'm trying to keep this email focused on high-level
>> concerns, I have to say that guaranteedlyLogged has got to be one of
>> the worst variable names I've ever seen, starting (but not ending)
>> with the fact that guaranteedly is not a word. I'm also tempted to
>> say that all of the wal_level=logical changes should be split out as
>> their own patch, separate from the decoding stuff. Personally, I
>> would find that much easier to review, although I admit it's less
>> critical here than for the RecentGlobalDataXmin stuff.
>
> I can do that again and it actually was that way in the past. But
> there's no user for it before the later patch and it's hard to
> understand the reasoning for the changed wal logging separately, that's
> why I merged it at some point.

OK. If I'm committing it, I'd prefer to handle that piece separately,
if possible.

>> - If XLOG_HEAP_INPLACE is not decoded, doesn't that mean that this
>> facility can't be used to replicate a system catalog table? Is that a
>> restriction we should enforce/document somehow?
>
> Currently catalog tables aren't replicated, yes. They simply are skipped
> during decoding. XLOG_HEAP_INPLACE isn't the primary reason for that
> though.
>
> Do you see a usecase for it?

I can imagine someone wanting to do it, but I think we can live with
it not being supported.

>> - It still bothers me that we're going to have mandatory slots for
>> logical replication and no slots for physical replication. Why are
>> slots mandatory in one case and not even allowable in the other? Is
>> it sensible to think about slotless logical replication - or maybe I
>> should say, is it any LESS sensible than slotless physical
>> replication?
>
> Well, as you know, I do want to have slots for physical replication as
> well. But there actually is a fundamental difference why we need it for
> logical rep and not for physical: In physical replication, if the xmin
> progresses too far, client queries will be cancelled. Annoying but not
> fatal. In logical replication we will not be able to continue
> replicating since we cannot decode the WAL stream without a valid
> catalog snapshot. If xmin already has progressed too far the tuples
> won't be there anymore.
>
> If people think this needs to be a general facility from the start, I
> can be convinced that way, but I think there's so much to discuss around
> the semantics and different usecases that I'd much prefer to discuss
> that later.

I'm worried that if we don't know how the physical replication slots
are going to work, they'll end up being randomly different from the
logical replication slots, and that'll be an API wart which we'll have
a hard time getting rid of later.

>> - What is the purpose of (Un)SuspendDecodingSnapshots? It seems that
>> should be explained somewhere. I have my doubts about how safe that
>> is.
>
> I'll document the details if they aren't right now. Consider what
> happens if somebody does something like: "VACUUM FULL pg_am;". If we
> were to build the relation descriptor of pg_am in an "historical
> snapshot", as you coin it, we'd have the wrong filenode in there. And
> consequently any future lookups in pg_am will open a file that doesn't
> exist.
> That problem only exist for non-nailed relations that are accessed
> during decoding.

But if it's some user table flagged with the terribly-named
treat_as_catalog_table flag, then they could have not only changed the
relfilenode but also the tupledesc. And then you can't just wave your
hands at the problem.

>> And I definitely think that SetupDecodingSnapshots() is not OK.
>> Overwriting the satisfies functions in static pointers may be a great
>> way to make sure you've covered all bases during development, but I
>> can't see us wanting that ugliness in the official sources.
>
> Yes, I don't like it either. I am not sure what to replace it with
> though. It's easy enough to fit something in GetCatalogSnapshot() and I
> don't have a problem with that, but I am less happy with adding code
> like that to GetSnapshotData() for callers that use explicit snapshots.

I'm not sure exactly what a good solution would like, either. I just
think this isn't it. :-)

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

Hi,

On 2013-09-19 12:05:35 -0400, Robert Haas wrote:
> No question. I'm not saying that that optimization shouldn't go in
> right after the main patch does, but IMHO right now there are too many
> things going in the 0004 patch to discuss them all simultaneously.
> I'd like to find a way of splitting this up that will let us
> block-and-tackle individual pieces of it, even we end up committing
> them all one right after the other.

Fine with me. I was critized for splitting up stuff too much before ;)

Expect a finer-grained series.

> But that raises an interesting question: why is the overhead so bad?
> I mean, this shouldn't be any worse than having a series of
> transactions running concurrently with pgbench that take a snapshot
> and hold it for as long as it takes the decoding process to decode the
> most-recently committed transaction.

Pgbench really slows down scarily if there are some slightly longer
running transactions around...

> Is the issue here that we can't
> advance xmin until we've fsync'd the fruits of decoding down to disk?

Basically yes. We only advance the xmin of the slot so far that we could
still build a valid snapshot to decode the first transaction not
confirmed to have been synced to disk by the client.

> If so, that's mighty painful. But we'd really only need to hold back
> xmin in that situation when some catalog change has occurred
> meanwhile, which for pgbench means never. So something seems fishy
> here.

It's less simple than that. We need to protect against concurrent DDL
producing deleted rows that we will still need. We need
HeapTupleStisfiesVacuum() to return HEAPTUPLE_RECENTLY_DEAD not
HEAPTUPLE_DEAD for such rows, right?
The way to do that is to guarantee that if
TransactionIdDidCommit(xmax) is true, TransactionIdPrecedes(xmax, OldestXmin) is also true.
So, we need to peg OldestXmin (as passed to HTSV) to the xid of the
oldest transaction we're still decoding.

I am not sure how you could do that iff somewhere in the future DDL has
started since there's no interlock preventing anyone against doing so.

> >> - It still bothers me that we're going to have mandatory slots for
> >> logical replication and no slots for physical replication.

> > If people think this needs to be a general facility from the start, I
> > can be convinced that way, but I think there's so much to discuss around
> > the semantics and different usecases that I'd much prefer to discuss
> > that later.
>
> I'm worried that if we don't know how the physical replication slots
> are going to work, they'll end up being randomly different from the
> logical replication slots, and that'll be an API wart which we'll have
> a hard time getting rid of later.

Hm. I actually think that minus some s/Logical//g and a mv won't be much
need to change on the slot interface itself.

What we need for physical rep is basically to a) store the position up
to where the primary has fsynced the WAL b) store the xmin horizon the standby
currently has.
Sure, we can store more stats (most of pg_stat_replication, perhaps some
more) but that's not functionally critical and not hard to extend.

The points I find daunting are the semantics, like:
* How do we control whether a standby is allowed prevent WAL file
removal. What if archiving is configured?
* How do we control whether a standby is allowed to peg xmin?
* How long do we peg an xmin/wal file removal if the standby is gone
* How does the userinterface look to remove a slot if a standby is gone
* How do we decide/control which commands use a slot in which cases?

> >> - What is the purpose of (Un)SuspendDecodingSnapshots? It seems that
> >> should be explained somewhere. I have my doubts about how safe that
> >> is.
> >
> > I'll document the details if they aren't right now. Consider what
> > happens if somebody does something like: "VACUUM FULL pg_am;". If we
> > were to build the relation descriptor of pg_am in an "historical
> > snapshot", as you coin it, we'd have the wrong filenode in there. And
> > consequently any future lookups in pg_am will open a file that doesn't
> > exist.
> > That problem only exist for non-nailed relations that are accessed
> > during decoding.
>
> But if it's some user table flagged with the terribly-named
> treat_as_catalog_table flag, then they could have not only changed the
> relfilenode but also the tupledesc. And then you can't just wave your
> hands at the problem.

Heh. Well cought.

There's a comment about that somewhere... Those are problematic, my plan
so far is to throw my hands up and forbid alter tables that rewrite
those.

I know you don't like that flag and especially it's name. I am open to
suggestions to a) rename it b) find a better solution. I am pretty sure
a) is possible but I have severe doubts about any realistic b).

> > Yes, I don't like it either. I am not sure what to replace it with
> > though. It's easy enough to fit something in GetCatalogSnapshot() and I
> > don't have a problem with that, but I am less happy with adding code
> > like that to GetSnapshotData() for callers that use explicit snapshots.
>
> I'm not sure exactly what a good solution would like, either. I just
> think this isn't it. :-)

I know that feeling ;)

Greetings,

Andres Freund

On Thu, Sep 19, 2013 at 7:43 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> More generally, the thing that bugs me about this approach is that
>> logical replication is not really special, but what you've done here
>> MAKES it special. There are plenty of other situations where we are
>> too aggressive about holding back xmin. A single-statement
>> transaction that selects from a single table holds back xmin for the
>> entire cluster, and that is Very Bad. It would probably be unfair to
>> say, well, you have to solve that problem first. But on the other
>> hand, how do we know that the approach you've adopted here isn't going
>> to make the more general problem harder to solve? It seems invasive
>> at a pretty low level.

I agree that it's invasive, but I am doubtful that pegging the xmin in
a more granular fashion precludes this kind of optimization. We might
have to generalize what Andres has done, which could mean eventually
throwing it out and starting from scratch, but I have a hard time
seeing how that implies an appreciable cost above solving the general
problem first (now that Andres has already implemented the
RecentGlobalDataXmin thing). As I'm sure you appreciate, the cost of
doing the opposite - of solving the general problem first - may be
huge: waiting another release for logical changeset generation.

> The reason why I think it's actually different is that the user actually
> has control over how long transactions are running on the primary. They
> don't really control how fast a replication consumer consumes and how
> often it sends feedback messages.

Right. That's about what I said last year.

I find the following analogy useful: A logical changeset generation
implementation without RecentGlobalDataXmin is kind of like an
old-fashioned nuclear reactor, like the one they had at Chernobyl.
Engineers have to actively work in order to prevent it from
overheating. However, an implementation with RecentGlobalDataXmin is
like a modern, much safer nuclear reactor. Engineers have to actively
work to keep the reactor heated. Which is to say, with
RecentGlobalDataXmin a standby that dies cannot bloat the master too
much (almost as with hot_standby_feedback - that too requires active
participation from the standby to do harm to the master). Without
RecentGlobalDataXmin, the core system and the plugin at the very least
need to worry about that case when a standby dies.

I have a little bit of feedback that I forgot to mention in my earlier
reviews, because I thought it was too trivial then: something about
the name pg_receivellog annoys me in a way that the name
pg_receivexlog does not. Specifically, it looks like someone meant to
type pg_receivelog but fat-fingered it.

--
Peter Geoghegan

On 09/20/2013 06:33 AM, Andres Freund wrote:
> Hi,
>

> The points I find daunting are the semantics, like:
> * How do we control whether a standby is allowed prevent WAL file
> removal. What if archiving is configured?
> * How do we control whether a standby is allowed to peg xmin?
> * How long do we peg an xmin/wal file removal if the standby is gone
> * How does the userinterface look to remove a slot if a standby is gone
> * How do we decide/control which commands use a slot in which cases?

I think we are going to want to be flexible enough to support users with
a couple of different points of use-cases.
* Some people will want to keep xmin pegged and prevent WAL removal so a
standby with a slot can always catch up, and wi
* Most people will want to say keep X megabytes of WA (if needed by a
behind slot) and keep xmin pegged so that the WAL can be consumed by a
logical plugin.

I can see us also implementing a restore_command that the walsender
could use to get archived segments but for logical replication xmin
would still need to be low enough

I don't think the current patch set is incompatible with us later
implementing any of the above. I'd rather see us focus on getting the
core functionality committed and worry about a good interface for
managing slots later.

> Greetings, Andres Freund

Steve

On 2013-09-20 14:15:23 -0700, Peter Geoghegan wrote:
> I have a little bit of feedback that I forgot to mention in my earlier
> reviews, because I thought it was too trivial then: something about
> the name pg_receivellog annoys me in a way that the name
> pg_receivexlog does not. Specifically, it looks like someone meant to
> type pg_receivelog but fat-fingered it.

Yes, you're not the first to dislike it (including me).

pg_receivelogical? Protest now or forever hold your peace.

Greetings,

Andres Freund

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

On Mon, Sep 23, 2013 at 1:46 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> pg_receivelogical? Protest now or forever hold your peace.

I was thinking pg_receiveloglog, but that works just as well.

--
Peter Geoghegan

Andres Freund escribió:
> On 2013-09-20 14:15:23 -0700, Peter Geoghegan wrote:
> > I have a little bit of feedback that I forgot to mention in my earlier
> > reviews, because I thought it was too trivial then: something about
> > the name pg_receivellog annoys me in a way that the name
> > pg_receivexlog does not. Specifically, it looks like someone meant to
> > type pg_receivelog but fat-fingered it.
>
> Yes, you're not the first to dislike it (including me).
>
> pg_receivelogical? Protest now or forever hold your peace.

I had proposed pg_recvlogical

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

On 2013-09-23 13:47:05 -0300, Alvaro Herrera wrote:
> Andres Freund escribió:
> > On 2013-09-20 14:15:23 -0700, Peter Geoghegan wrote:
> > > I have a little bit of feedback that I forgot to mention in my earlier
> > > reviews, because I thought it was too trivial then: something about
> > > the name pg_receivellog annoys me in a way that the name
> > > pg_receivexlog does not. Specifically, it looks like someone meant to
> > > type pg_receivelog but fat-fingered it.
> >
> > Yes, you're not the first to dislike it (including me).
> >
> > pg_receivelogical? Protest now or forever hold your peace.
>
> I had proposed pg_recvlogical

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

Greetings,

Andres Freund

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

On Mon, Sep 23, 2013 at 9:54 AM, Andres Freund <andres(at)2ndquadrant(dot)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

--
Peter Geoghegan

Andres Freund escribió:
> On 2013-09-23 13:47:05 -0300, Alvaro Herrera wrote:

> > I had proposed pg_recvlogical
>
> 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"?

Well. What are the principles we want to follow when choosing a name?
Is consistency the first and foremost consideration? To me, that names
are exactly consistent is not all that relevant; I prefer a shorter name
if it embodies all it means. For that reason I didn't like the
"receiveloglog" suggestion: it's not clear what are the two "log" bits.
To me this suggests that "logical" should not be shortened. But the
"recv" thing is clear to be "receive", isn't it? Enough that it can be
shortened without loss of meaning.

If we consider consistency in naming of tools is uber-important, well,
obviously my proposal is dead.

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

On 9/23/13 12:54 PM, Andres Freund 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"?

It's consistent because they are the same length!

(Obviously, this would severely restrict future tool naming.)

In all seriousness, I like this naming best so far.

On Mon, Sep 23, 2013 at 1:11 PM, Alvaro Herrera
<alvherre(at)2ndquadrant(dot)com> wrote:
> Andres Freund escribió:
>> On 2013-09-23 13:47:05 -0300, Alvaro Herrera wrote:
>
>> > I had proposed pg_recvlogical
>>
>> 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"?
>
> Well. What are the principles we want to follow when choosing a name?
> Is consistency the first and foremost consideration? To me, that names
> are exactly consistent is not all that relevant; I prefer a shorter name
> if it embodies all it means. For that reason I didn't like the
> "receiveloglog" suggestion: it's not clear what are the two "log" bits.
> To me this suggests that "logical" should not be shortened. But the
> "recv" thing is clear to be "receive", isn't it? Enough that it can be
> shortened without loss of meaning.
>
> If we consider consistency in naming of tools is uber-important, well,
> obviously my proposal is dead.

What exactly is the purpose of this tool? My impression is that the
"output" of logical replication is a series of function calls to a
logical replication plugin, but does that plugin necessarily have to
produce an output format that gets streamed to a client via a tool
like this? For example, for replication, I'd think you might want the
plugin to connect to a remote database and directly shove the data in;
for materialized views, we might like to push the changes into delta
relations within the source database. In either case, there's no
particular need for any sort of client at all, and in fact it would be
much better if none were required. The existence of a tool like
pg_receivellog seems to presuppose that the goal is spit out logical
change records as text, but I'm not sure that's actually going to be a
very common thing to want to do...

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

On Mon, Sep 23, 2013 at 8:12 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> The existence of a tool like
> pg_receivellog seems to presuppose that the goal is spit out logical
> change records as text, but I'm not sure that's actually going to be a
> very common thing to want to do...

Sure, but I think it's still worth having, for debugging purposes and
so on. Perhaps the incorrect presupposition is that it deserves to
live in /bin and not /contrib. Also, even though the tool is
derivative of pg_receivexlog, its reason for existing is sufficiently
different that maybe it deserves an entirely distinct name. On the
other hand, precisely because it's derivative of
receivelog/pg_receivexlog, it kind of makes sense to group them
together like that. So I don't know.

--
Peter Geoghegan

On 2013-09-23 23:12:53 -0400, Robert Haas wrote:
> What exactly is the purpose of this tool? My impression is that the
> "output" of logical replication is a series of function calls to a
> logical replication plugin, but does that plugin necessarily have to
> produce an output format that gets streamed to a client via a tool
> like this?

There needs to be a client acking the reception of the data in some
form. There's currently two output methods, SQL and walstreamer, but
there easily could be further, it's basically two functions you have
write.

There are several reasons I think the tool is useful, starting with the
fact that it makes the initial use of the feature easier. Writing a
client for CopyBoth messages wrapping 'w' style binary messages, with the
correct select() loop isn't exactly trivial. I also think it's actually
useful in "real" scenarios where you want to ship the data to a
remote system for auditing purposes.

> For example, for replication, I'd think you might want the
> plugin to connect to a remote database and directly shove the data in;

That sounds like a bad idea to me. If you pull the data from the remote
side, you get the data in a streaming fashion and the latency sensitive
part of issuing statements to your local database is done locally.
Doing things synchronously like that also makes it way harder to use
synchronous_commit = off on the remote side, which is a tremendous
efficiency win.

If somebody needs something like this, e.g. because they want to
replicate into hundreds of shards depending on some key or such, the
question I don't know is how to actually initiate the
streaming. Somebody would need to start the logical decoding.

> for materialized views, we might like to push the changes into delta
> relations within the source database.

Yes, that's not a bad usecase and I think the only thing missing to use
output plugins that way is a convenient function to tell up to where
data has been received (aka synced to disk, aka applied).

> In either case, there's no
> particular need for any sort of client at all, and in fact it would be
> much better if none were required. The existence of a tool like
> pg_receivellog seems to presuppose that the goal is spit out logical
> change records as text, but I'm not sure that's actually going to be a
> very common thing to want to do...

It doesn't really rely on anything being text - I've used it with a
binary plugin without problems. Obviously you might not want to use -f -
but an actual file instead...

Greetings,

Andres Freund

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

On Tue, Sep 24, 2013 at 4:15 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> There needs to be a client acking the reception of the data in some
> form. There's currently two output methods, SQL and walstreamer, but
> there easily could be further, it's basically two functions you have
> write.
>
> There are several reasons I think the tool is useful, starting with the
> fact that it makes the initial use of the feature easier. Writing a
> client for CopyBoth messages wrapping 'w' style binary messages, with the
> correct select() loop isn't exactly trivial. I also think it's actually
> useful in "real" scenarios where you want to ship the data to a
> remote system for auditing purposes.

I have two basic points here:

- Requiring a client is a short-sighted design. There's no reason we
shouldn't *support* having a client, but IMHO it shouldn't be the only
way to use the feature.

- Suppose that you use pg_receivellog (or whatever we decide to call
it) to suck down logical replication messages. What exactly are you
going to do with that data once you've got it? In the case of
pg_receivexlog it's quite obvious what you will do with the received
files: you'll store them in archive of some kind and maybe eventually
use them for archive recovery, streaming replication, or PITR. But
the answer here is a lot less obvious, at least to me.

>> For example, for replication, I'd think you might want the
>> plugin to connect to a remote database and directly shove the data in;
>
> That sounds like a bad idea to me. If you pull the data from the remote
> side, you get the data in a streaming fashion and the latency sensitive
> part of issuing statements to your local database is done locally.
> Doing things synchronously like that also makes it way harder to use
> synchronous_commit = off on the remote side, which is a tremendous
> efficiency win.

This sounds like the voice of experience talking, so I won't argue too
much, but I don't think it's central to my point. And anyhow, even if
it is a bad idea, that doesn't mean someone won't want to do it. :-)

> If somebody needs something like this, e.g. because they want to
> replicate into hundreds of shards depending on some key or such, the
> question I don't know is how to actually initiate the
> streaming. Somebody would need to start the logical decoding.

Sounds like a job for a background worker. It would be pretty swell
if you could write a background worker that connects to a logical
replication slot and then does whatever.

>> for materialized views, we might like to push the changes into delta
>> relations within the source database.
>
> Yes, that's not a bad usecase and I think the only thing missing to use
> output plugins that way is a convenient function to tell up to where
> data has been received (aka synced to disk, aka applied).

Yes. It feels to me (and I only work here) like the job of the output
plugin ought to be to put the data somewhere, and the replication code
shouldn't make too many assumptions about where it's actually going.

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

On 2013-09-24 11:04:06 -0400, Robert Haas wrote:
> - Requiring a client is a short-sighted design. There's no reason we
> shouldn't *support* having a client, but IMHO it shouldn't be the only
> way to use the feature.

There really aren't many limitations preventing you from doing anything
else.

> - Suppose that you use pg_receivellog (or whatever we decide to call
> it) to suck down logical replication messages. What exactly are you
> going to do with that data once you've got it? In the case of
> pg_receivexlog it's quite obvious what you will do with the received
> files: you'll store them in archive of some kind and maybe eventually
> use them for archive recovery, streaming replication, or PITR. But
> the answer here is a lot less obvious, at least to me.

Well, it's not like it's going to be the only client. But it's a useful
one. I don't see this as an argument against pg_receivelogical? Most
sites don't use pg_receivexlog either.
Not having a consumer of the walsender interface included sounds like a
bad idea to me, even if it were only useful for testing. Now, you could
argue it should be in /contrib - and I wouldn't argue against that
except it shares code with the rest of src/bin/pg_basebackup.

> > If somebody needs something like this, e.g. because they want to
> > replicate into hundreds of shards depending on some key or such, the
> > question I don't know is how to actually initiate the
> > streaming. Somebody would need to start the logical decoding.

> Sounds like a job for a background worker. It would be pretty swell
> if you could write a background worker that connects to a logical
> replication slot and then does whatever.

That's already possible. In that case you don't have to connect to a
walsender, although doing so would give you some parallelism, one
decoding the data, the other processing it ;).

There's one usecase I do not forsee decoupling from the walsender
interface this release though - synchronous logical replication. There
currently is no code changes required to make sync rep work for this,
and decoupling sync rep from walsender is too much to bite off in one
go.

> >> for materialized views, we might like to push the changes into delta
> >> relations within the source database.
> >
> > Yes, that's not a bad usecase and I think the only thing missing to use
> > output plugins that way is a convenient function to tell up to where
> > data has been received (aka synced to disk, aka applied).
>
> Yes. It feels to me (and I only work here) like the job of the output
> plugin ought to be to put the data somewhere, and the replication code
> shouldn't make too many assumptions about where it's actually going.

The output plugin just has two functions it calls to send out data,
'prepare_write' and 'write'. The callsite has to provide those
callbacks. Two are included. walsender and an SQL SRF.

Check the 'test_logical_decoding commit, it includes the SQL consumer.

Greetings,

Andres Freund

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

On 09/24/2013 11:21 AM, Andres Freund wrote:
> Not having a consumer of the walsender interface included sounds like a
> bad idea to me, even if it were only useful for testing. Now, you could
> argue it should be in /contrib - and I wouldn't argue against that
> except it shares code with the rest of src/bin/pg_basebackup.

+1 on pg_receivellog (or whatever better name we pick) being somewhere.
I found the pg_receivellog code very useful as an example and for
debugging/development purposes.
It isn't something that I see useful for the average user and I think
the use-cases it meets are closer to other things we usually put in /contrib

Steve

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

The call stack is

HeapTupleSatisfiesMVCCDuringDecoding
heap_hot_search_buffer
index_fetch_heap
index_getnext
systable_getnext
RelidByRelfilenode
ReorderBufferCommit
DecodeCommit
.
.
.

I am working off something based on your version
e0acfeace6d695c229efd5d78041a1b734583431

Any ideas?

> Greetings,
>
> Andres Freund
>
>
>

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?

> Any ideas?

I'll look into it. Could you provide any context to what youre doing
that's being decoded?

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(at)github(dot)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
>

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(at)github(dot)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
>>
>
>
>

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

On 27 September 2013 16:14, Andres Freund <andres(at)2ndquadrant(dot)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

On 2013-09-27 16:35:53 +0100, Thom Brown wrote:
> On 27 September 2013 16:14, Andres Freund <andres(at)2ndquadrant(dot)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

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(at)(dot)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
>>>
>>
>>
>>
>
>
>

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(at)(dot)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

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

>

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(at)(dot)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(at)entry=0x8cf400 "!(ent->cmin ==
change->tuplecid.cmin)", errorType=errorType(at)entry=0x7ab830
"FailedAssertion",
fileName=fileName(at)entry=0x8cecc3 "reorderbuffer.c",
lineNumber=lineNumber(at)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
>

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

Andres Freund <andres(at)2ndquadrant(dot)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

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

Review comments on 0004:

- In heap_insert and heap_multi_insert, please rewrite the following
comment for clarity: "add record for the buffer without actual content
thats removed if fpw is done for that buffer".
- In heap_delete, the assignment to need_tuple_data() need not
separately check RelationNeedsWAL(), as RelationIsLogicallyLogged()
does that.
- 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.
- MarkCurrentTransactionIdLoggedIfAny has superfluous braces.
- AssignTransactionId changes "Mustn't" to "May not", which seems like
an entirely pointless change.
- You've removed a blank line just before IsSystemRelation; this is an
unnecessary whitespace change.
- Do none of the callers of IsSystemRelation() care about the fact
that you've considerably changed the semantics?
- RelationIsDoingTimetravel is still a crappy name. How about
RelationRequiredForLogicalDecoding? And maybe the reloption
treat_as_catalog_table can become required_for_logical_decoding.
- I don't understand the comment in xl_heap_new_cid to the effect that
the combocid isn't needed for decoding. How not?
- xlogreader.h includes an additional header with no other changes.
Doesn't seem right.
- relcache.h has a cuddled curly brace.

Review comments on 0003:

I have no problem with caching the primary key in the relcache, or
with using that as the default key for logical decoding, but I'm
extremely uncomfortable with the fallback strategy when no primary key
exists. Choosing any old unique index that happens to present itself
as the primary key feels wrong to me. The choice of key is
user-visible. If we say, update the row with a = 1 to
(a,b,c)=(2,2,2), that's different than saying update the row with b =
1 to (a,b,c)=(2,2,2). Suppose the previous contents of the target
table are (a,b,c)=(1,2,3) and (a,b,c)=(2,1,4). You get different
answers depending on which you choose. I think multi-master
replication just isn't going to work unless the two sides agree on the
key, and I think you'll get strange conflicts unless that key is
chosen by the user according to their business logic.

In single-master replication, being able to pick the key is clearly
not essential for correctness, but it's still desirable, because if
the system picks the "wrong" key, the change stream will in the end
get the database to the right state, but it may do so by "turning one
record into a different one" from the user's perspective.

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.

What happens if new unique indexes are created or old ones dropped
while logical replication is running?

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

Hi,

On 2013-10-01 10:07:19 -0400, Robert Haas wrote:
> - AssignTransactionId changes "Mustn't" to "May not", which seems like
> an entirely pointless change.

It was "Musn't" before ;). I am not sure why I changed it to "May not"
instead of "Mustn't".

> - Do none of the callers of IsSystemRelation() care about the fact
> that you've considerably changed the semantics?

Afaics no. I think the semantics are actually consistent until somebody
manually creates a relation in pg_catalog (using allow_...). And in that
case the new semantics actually seem more useful.

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

Fine with me.

> - I don't understand the comment in xl_heap_new_cid to the effect that
> the combocid isn't needed for decoding. How not?

We don't use the combocid for antything - since we have the original
cmin/cmax, we can just use those and ignore the value of the combocid itself.

> - xlogreader.h includes an additional header with no other changes.
> Doesn't seem right.

Hm. I seem to remember having a reason for that, but for the heck can't
see it anymore...

> I have no problem with caching the primary key in the relcache, or
> with using that as the default key for logical decoding, but I'm
> extremely uncomfortable with the fallback strategy when no primary key
> exists. Choosing any old unique index that happens to present itself
> as the primary key feels wrong to me.
> [stuff I don't disagree with]

People lobbied vigorously to allow candidate keys before. I personally
would never want to use anything but an actual primary key for
replication, but there's other usecases than replication.

I think it's going to be the domain of the replication solution to
enforce the presence of primary keys. I.e. they should (be able to) use
event triggers or somesuch to enforce it...

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

I'd be fine with that, but I am also not particularly interested in it
because I personally don't see much of a usecase.
For replication ISTM the only case where there would be no primary key
is a) initial load b) replacing the primary key by another index.

> What happens if new unique indexes are created or old ones dropped
> while logical replication is running?

Should just work, but I'll make sure the tests cover this.

The output plugin needs to lookup the current index used, and it will
use a consistent syscache state and thus will find the same index.
In bdr the output plugin simply includes the name of the index used in
the replication stream to make sure things are somewhat consistent.

Will fix or think about the rest.

Thanks,

Andres Freund

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

On Tue, Oct 1, 2013 at 10:31 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I have no problem with caching the primary key in the relcache, or
>> with using that as the default key for logical decoding, but I'm
>> extremely uncomfortable with the fallback strategy when no primary key
>> exists. Choosing any old unique index that happens to present itself
>> as the primary key feels wrong to me.
>> [stuff I don't disagree with]
>
> People lobbied vigorously to allow candidate keys before. I personally
> would never want to use anything but an actual primary key for
> replication, but there's other usecases than replication.

I like allowing candidate keys; I just don't like assuming that any
old one we select will be as good as any other.

>> 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.
>
> I'd be fine with that, but I am also not particularly interested in it
> because I personally don't see much of a usecase.
> For replication ISTM the only case where there would be no primary key
> is a) initial load b) replacing the primary key by another index.

The latter is the case I'd be principally concerned about. I once had
to change the columns that formed the key for a table being used in a
production web application; fortunately, it has traditionally not
mattered much whether a unique index is the primary key, so creating a
new unique index and dropping the old primary key was good enough.
But I would have wanted to control the point at which we changed our
notion of what the candidate key was, I think.

One other thought: you could just log the whole old tuple if there's
no key available. That would let this work on tables that don't have
indexes. Replaying the changes might be horribly complex and slow,
but extracting them would work. If a replication plugin got <old
tuple, new tuple> with no information on keys, it could find *a* tuple
(not all tuples) that match the old tuple exactly and update each
column to the value from new tuple. From a correctness point of view,
there's no issue there; it's all about efficiency. But the user can
solve that problem whenever they like by indexing the destination
table. It need not even be a unique index, so long as it's reasonably
selective.

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

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.

Ok, I had to think a bit, but now I remember why I think these changes
are not really problem: Neither the addition of keys nor candidate keys
will add any additional comparisons since the columns compared for
candidate keys are a subset of the set of key columns which in turn are a
subset of the columns checked for HOT. Right?

Greetings,

Andres Freund

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

On 09/30/2013 06:44 PM, Andres Freund wrote:
> Hi,
>
> The series from friday was a bit too buggy - obviously I was too
> tired. So here's a new one:

With this series I've also noticed
#2 0x00000000007741a7 in ExceptionalCondition (
conditionName=conditionName(at)entry=0x7c2908 "!(!(tuple->t_infomask &
0x1000))", errorType=errorType(at)entry=0x7acc70 "FailedAssertion",
fileName=fileName(at)entry=0x91767e "tqual.c",
lineNumber=lineNumber(at)entry=1608) at assert.c:54
54 abort();

0x00000000007a4432 in HeapTupleSatisfiesMVCCDuringDecoding (
htup=0x10bfe48, snapshot=0x108b3d8, buffer=310) at tqual.c:1608
#4 0x000000000049d6b7 in heap_hot_search_buffer (tid=tid(at)entry=0x10bfe4c,
relation=0x7fbebbcd89c0, buffer=310, snapshot=0x10bfda0,
heapTuple=heapTuple(at)entry=0x10bfe48,
all_dead=all_dead(at)entry=0x7fff4aa3866f "\001\370\375\v\001",
first_call=1 '\001') at heapam.c:1756
#5 0x00000000004a8174 in index_fetch_heap (scan=scan(at)entry=0x10bfdf8)
at indexam.c:539
#6 0x00000000004a82a8 in index_getnext (scan=0x10bfdf8,
direction=direction(at)entry=ForwardScanDirection) at indexam.c:622
#7 0x00000000004a6fa9 in systable_getnext (sysscan=sysscan(at)entry=0x10bfd48)
at genam.c:343
#8 0x000000000076df40 in RelidByRelfilenode (reltablespace=0,
relfilenode=529775) at relfilenodemap.c:214
---Type <return> to continue, or q <return> to quit---
#9 0x0000000000664ad7 in ReorderBufferCommit (rb=0x1082d98,
xid=<optimized out>, commit_lsn=4638756800, end_lsn=<optimized out>,
commit_time=commit_time(at)entry=433970378426176) at reorderbuffer.c:1320

In addition to some of the other ones I've posted about.

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

On Tue, Oct 1, 2013 at 1:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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.
>
> Ok, I had to think a bit, but now I remember why I think these changes
> are not really problem: Neither the addition of keys nor candidate keys
> will add any additional comparisons since the columns compared for
> candidate keys are a subset of the set of key columns which in turn are a
> subset of the columns checked for HOT. Right?

TBH, my primary concern was with maintainability more than performance.

On performance, I think any time you add code it's going to cost
somehow. However, it might not be enough to care about.

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

On 2013-10-02 10:56:38 -0400, Robert Haas wrote:
> On Tue, Oct 1, 2013 at 1:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > 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.
> >
> > Ok, I had to think a bit, but now I remember why I think these changes
> > are not really problem: Neither the addition of keys nor candidate keys
> > will add any additional comparisons since the columns compared for
> > candidate keys are a subset of the set of key columns which in turn are a
> > subset of the columns checked for HOT. Right?
>
> TBH, my primary concern was with maintainability more than performance.
>
> On performance, I think any time you add code it's going to cost
> somehow. However, it might not be enough to care about.

The easy alternative seems to be to call such a function multiple times
- which I think is prohibitive from a performance POV. More radically we
could simply compute the overall set/bitmap of differening columns and
then use bms_is_subset() to determine whether any index columns/key/ckey
columns changed. But that will do comparisons we don't do today...

Greetings,

Andres Freund

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

On Wed, Oct 2, 2013 at 11:05 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-02 10:56:38 -0400, Robert Haas wrote:
>> On Tue, Oct 1, 2013 at 1:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > 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.
>> >
>> > Ok, I had to think a bit, but now I remember why I think these changes
>> > are not really problem: Neither the addition of keys nor candidate keys
>> > will add any additional comparisons since the columns compared for
>> > candidate keys are a subset of the set of key columns which in turn are a
>> > subset of the columns checked for HOT. Right?
>>
>> TBH, my primary concern was with maintainability more than performance.
>>
>> On performance, I think any time you add code it's going to cost
>> somehow. However, it might not be enough to care about.
>
> The easy alternative seems to be to call such a function multiple times
> - which I think is prohibitive from a performance POV. More radically we
> could simply compute the overall set/bitmap of differening columns and
> then use bms_is_subset() to determine whether any index columns/key/ckey
> columns changed. But that will do comparisons we don't do today...

Yeah, there may be no better alternative to doing things as you've
done them here. It just looks grotty, so I was hoping we had a better
idea. Maybe not.

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

On 2013-10-02 11:06:59 -0400, Robert Haas wrote:
> On Wed, Oct 2, 2013 at 11:05 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-02 10:56:38 -0400, Robert Haas wrote:
> >> On Tue, Oct 1, 2013 at 1:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> > 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.
> >> >
> >> > Ok, I had to think a bit, but now I remember why I think these changes
> >> > are not really problem: Neither the addition of keys nor candidate keys
> >> > will add any additional comparisons since the columns compared for
> >> > candidate keys are a subset of the set of key columns which in turn are a
> >> > subset of the columns checked for HOT. Right?
> >>
> >> TBH, my primary concern was with maintainability more than performance.
> >>
> >> On performance, I think any time you add code it's going to cost
> >> somehow. However, it might not be enough to care about.
> >
> > The easy alternative seems to be to call such a function multiple times
> > - which I think is prohibitive from a performance POV. More radically we
> > could simply compute the overall set/bitmap of differening columns and
> > then use bms_is_subset() to determine whether any index columns/key/ckey
> > columns changed. But that will do comparisons we don't do today...
>
> Yeah, there may be no better alternative to doing things as you've
> done them here. It just looks grotty, so I was hoping we had a better
> idea. Maybe not.

Imo the code now looks easier to understand - which is not saying much -
than in 9.3/HEAD...

Greetings,

Andres Freund

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

On 2013-10-01 16:11:47 -0400, Steve Singer wrote:
> On 09/30/2013 06:44 PM, Andres Freund wrote:
> >Hi,
> >
> >The series from friday was a bit too buggy - obviously I was too
> >tired. So here's a new one:
>
> With this series I've also noticed
> #2 0x00000000007741a7 in ExceptionalCondition (
> conditionName=conditionName(at)entry=0x7c2908 "!(!(tuple->t_infomask &
> 0x1000))", errorType=errorType(at)entry=0x7acc70 "FailedAssertion",
> fileName=fileName(at)entry=0x91767e "tqual.c",
> lineNumber=lineNumber(at)entry=1608) at assert.c:54
> 54 abort();
>
>
> 0x00000000007a4432 in HeapTupleSatisfiesMVCCDuringDecoding (
> htup=0x10bfe48, snapshot=0x108b3d8, buffer=310) at tqual.c:1608
> #4 0x000000000049d6b7 in heap_hot_search_buffer (tid=tid(at)entry=0x10bfe4c,
> relation=0x7fbebbcd89c0, buffer=310, snapshot=0x10bfda0,
> heapTuple=heapTuple(at)entry=0x10bfe48,
> all_dead=all_dead(at)entry=0x7fff4aa3866f "\001\370\375\v\001",
> first_call=1 '\001') at heapam.c:1756
> #5 0x00000000004a8174 in index_fetch_heap (scan=scan(at)entry=0x10bfdf8)
> at indexam.c:539
> #6 0x00000000004a82a8 in index_getnext (scan=0x10bfdf8,
> direction=direction(at)entry=ForwardScanDirection) at indexam.c:622
> #7 0x00000000004a6fa9 in systable_getnext (sysscan=sysscan(at)entry=0x10bfd48)
> at genam.c:343
> #8 0x000000000076df40 in RelidByRelfilenode (reltablespace=0,
> relfilenode=529775) at relfilenodemap.c:214
> ---Type <return> to continue, or q <return> to quit---
> #9 0x0000000000664ad7 in ReorderBufferCommit (rb=0x1082d98,
> xid=<optimized out>, commit_lsn=4638756800, end_lsn=<optimized out>,
> commit_time=commit_time(at)entry=433970378426176) at reorderbuffer.c:1320

Does your code use SELECT FOR UPDATE/SHARE on system or treat_as_catalog
tables?

Greetings,

Andres Freund

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

On 10/03/2013 12:38 PM, Andres Freund wrote:
> Does your code use SELECT FOR UPDATE/SHARE on system or
> treat_as_catalog tables? Greetings, Andres Freund

Yes.
It declares sl_table and sl_sequence and sl_set as catalog.

It does a
SELECT ......
from @NAMESPACE(at)(dot)sl_table T, @NAMESPACE(at)(dot)sl_set S,
"pg_catalog".pg_class PGC, "pg_catalog".pg_namespace PGN,
"pg_catalog".pg_index PGX, "pg_catalog".pg_class PGXC
where ... for update

in the code being executed by the 'set add table'.

(We also do select for update commands in many other places during
cluster configuration commands)

On 2013-10-03 13:03:07 -0400, Steve Singer wrote:
> On 10/03/2013 12:38 PM, Andres Freund wrote:
> >Does your code use SELECT FOR UPDATE/SHARE on system or treat_as_catalog
> >tables? Greetings, Andres Freund
>
> Yes.
> It declares sl_table and sl_sequence and sl_set as catalog.
>
> It does a
> SELECT ......
> from @NAMESPACE(at)(dot)sl_table T, @NAMESPACE(at)(dot)sl_set S,
> "pg_catalog".pg_class PGC, "pg_catalog".pg_namespace PGN,
> "pg_catalog".pg_index PGX, "pg_catalog".pg_class PGXC
> where ... for update
>
> in the code being executed by the 'set add table'.
>
> (We also do select for update commands in many other places during cluster
> configuration commands)

Ok, there were a couple of bugs because I thought mxacts wouldn't need
to be supported. So far your testcase doesn't crash the database
anymore - it spews some internal errors though, so I am not sure if it's
entirely fixed for you.

Thanks for testing and helping!

I've pushed the changes to the git tree, they aren't squashed yet and
there's some further outstanding stuff, so I won't repost the series yet.

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

On 10/03/2013 04:00 PM, Andres Freund wrote:
> Ok, there were a couple of bugs because I thought mxacts wouldn't need
> to be supported. So far your testcase doesn't crash the database
> anymore - it spews some internal errors though, so I am not sure if
> it's entirely fixed for you. Thanks for testing and helping! I've
> pushed the changes to the git tree, they aren't squashed yet and
> there's some further outstanding stuff, so I won't repost the series
> yet. Greetings, Andres Freund
When I run your updated version (from friday, not what you posted today)
against a more recent version of my slony changes I can get the test
case to pass 2/3 'rd of the time. The failures are due to an issue in
slon itself that I need to fix.

I see lots of
0LOG: tx with subtxn 58836

but they seem harmless.

Thanks

On 2013-10-07 09:56:11 -0400, Steve Singer wrote:
> On 10/03/2013 04:00 PM, Andres Freund wrote:
> >Ok, there were a couple of bugs because I thought mxacts wouldn't need to
> >be supported. So far your testcase doesn't crash the database anymore - it
> >spews some internal errors though, so I am not sure if it's entirely fixed
> >for you. Thanks for testing and helping! I've pushed the changes to the
> >git tree, they aren't squashed yet and there's some further outstanding
> >stuff, so I won't repost the series yet. Greetings, Andres Freund
> When I run your updated version (from friday, not what you posted today)
> against a more recent version of my slony changes I can get the test case to
> pass 2/3 'rd of the time. The failures are due to an issue in slon itself
> that I need to fix.

Cool.

> I see lots of
> 0LOG: tx with subtxn 58836

Yes, those are completely harmless. And should, in fact, be removed. I
guess I should add the todo entry:
* make a pass over all elog/ereport an make sure they have the correct
log level et al.

Greetings,

Andres Freund

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

On Mon, Oct 7, 2013 at 9:32 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> - 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.

user_catalog_table is a pretty good description, but should we worry
about the fact that logical replication isn't mentioned in there
anywhere?

In what way do you feel that it's more clear to say *accessible
during* rather than *required for* logical decoding?

I was trying to make the naming consistent; i.e. if we have
RelationRequiredForLogicalDecoding then name the option to match.

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

I'm OK with defaulting to the primary key if there is one, but I think
that no other candidate key should be entertained unless the user
configures it. I think the behavior we get without that will be just
too weird. We could use the same logic you're proposing here for
CLUSTER, too, but we don't; that's because we've (IMHO, rightly)
decided that the choice of index is too important to be left to
chance.

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

On 10/07/2013 09:32 AM, Andres Freund wrote:
> 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

What is your current plan for decoding sequence updates? Is this
something that you were going to hold-off on supporting till a future
version? ( know this was discussed a while ago but I don't remember
where it stands now)

From a Slony point of view this isn't a big deal, I can continue to
capture sequence changes in sl_seqlog when I create each SYNC event and
then just replicate the INSERT statements in sl_seqlog via logical
decoding. I can see why someone building a replication system not based
on the concept of a SYNC would have a harder time with this.

I am guessing we would want to pass sequence operations to the plugins
as we encounter the WAL for them out-of-band of any transaction. This
would mean that a set of operations like

begin;
insert into a (id) values(4);
insert into a (id) values(nextval('some_seq'));
commit

would be replayed on the replicas as
setval('some_seq',100);
begin;
insert into a (id) values (4);
insert into a (id) values (100);
commit;

On 2013-10-08 15:02:39 -0400, Steve Singer wrote:
> On 10/07/2013 09:32 AM, Andres Freund wrote:
> >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
>
> What is your current plan for decoding sequence updates? Is this something
> that you were going to hold-off on supporting till a future version? ( know
> this was discussed a while ago but I don't remember where it stands now)

I don't plan to implement it as part of this - the optimizations in
sequences make it really unsuitable for that (nontransaction, allocated
in bulk, ...).
Simon had previously posted about "sequence AMs", and I have a prototype
patch that implements that concept (which needs considerable cleanup). I
plan to post about it whenever this is finished.

I think many replication solutions that care about sequences in a
nontrivial will want to implement their own sequence logic anyway, so I
think that's not a bad path.

Greetings,

Andres Freund

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

On 2013-10-08 12:20:22 -0400, Robert Haas wrote:
> On Mon, Oct 7, 2013 at 9:32 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> - 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.
>
> user_catalog_table is a pretty good description, but should we worry
> about the fact that logical replication isn't mentioned in there
> anywhere?

I personally don't worry about it, although I see why somebody could.

> In what way do you feel that it's more clear to say *accessible
> during* rather than *required for* logical decoding?

Because "required for" can easily be understood that you need to set it
if you want a table's changes to be replicated. Which is not the case...

> I was trying to make the naming consistent; i.e. if we have
> RelationRequiredForLogicalDecoding then name the option to match.

Maybe this should be RelationAccessibleInLogicalDecoding() then - that
seems like a better description anyway?

> >> 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?
>
> I'm OK with defaulting to the primary key if there is one, but I think
> that no other candidate key should be entertained unless the user
> configures it. I think the behavior we get without that will be just
> too weird. We could use the same logic you're proposing here for
> CLUSTER, too, but we don't; that's because we've (IMHO, rightly)
> decided that the choice of index is too important to be left to
> chance.

I don't understand why this would be a good path. If you DELETE/UPDATE
and you don't have a primary key you get something that definitely
identifies the row with the current behaviour. It might not be the best
thing, but it sure is better than nothing. E.g. for auditing it's
probably quite sufficient to just use any of the candidate keys if
there (temporarily) is no primary key.
If you implement a replication solution and don't want that behaviour
there, you are free to guard against it there - which is a good thing
to do.

Greetings,

Andres Freund

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

On Mon, Sep 30, 2013 at 6:44 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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.

I spent some time looking at the sample plugin (patch 9/12). Here are
some review comments:

- I think that the decoding plugin interface should work more like the
foreign data wrapper interface. Instead of using pg_dlsym to look up
fixed names, I think there should be a struct of function pointers
that gets filled in and registered somehow.

- pg_decode_init() only warns when it encounters an unknown option.
An error seems more appropriate.

- Still wondering how we'll use this from a bgworker.

- The output format doesn't look very machine-parseable. I really
think we ought to provide something that is. Maybe a CSV-like format,
or maybe something else, but I don't see why someone who wants to do
change logging should be forced to write and install C code. If
something like Bucardo can run on an unmodified system and extract
change-sets this way without needing a .so file, that's going to be a
huge win for usability.

Other than that, I don't have too many concerns about the plugin
interface. I think it provides useful flexibility and it generally
seems well-designed. I hope in the future we'll be able to decode
transactions on the fly instead of waiting until commit time, but I've
resigned myself to the fact that we may not get that in version one.

More generally on this patch set, if I'm going to be committing any of
this, I'd prefer to start with what is currently patches 3 and 4, once
we reach agreement on those.

Are we hoping to get any of this committed for this CF? If so, let's
make a plan to get that done; time is short. If not, let's update the
CF app accordingly.

Thanks,

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

Hi Robert,

On 2013-10-09 14:49:46 -0400, Robert Haas wrote:
> I spent some time looking at the sample plugin (patch 9/12). Here are
> some review comments:
>
> - I think that the decoding plugin interface should work more like the
> foreign data wrapper interface. Instead of using pg_dlsym to look up
> fixed names, I think there should be a struct of function pointers
> that gets filled in and registered somehow.

You mean something like CREATE OUTPUT PLUGIN registering a function with
an INTERNAL return value returning a filled struct? I thought about
that, but it seemed more complex. Happy to change it though if it's
preferred.

> - pg_decode_init() only warns when it encounters an unknown option.
> An error seems more appropriate.

Fine with me. I think I just made it a warning because I wanted to
experiment with options.

> - Still wondering how we'll use this from a bgworker.

Simplified code to consume data:

LogicalDecodingReAcquireSlot(NameStr(*name));
ctx = CreateLogicalDecodingContext(MyLogicalDecodingSlot, false /* not initial call */,
MyLogicalDecodingSlot->confirmed_flush,
options,
logical_read_local_xlog_page,
LogicalOutputPrepareWrite,
LogicalOutputWrite);
...
while (true)
{
XLogRecord *record;
char *errm = NULL;

record = XLogReadRecord(ctx->reader, startptr, &errm);
...
DecodeRecordIntoReorderBuffer(ctx, &buf);
}

/* at the end or better ever commit or such */
LogicalConfirmReceivedLocation(/* whatever you consumed */);

LogicalDecodingReleaseSlot();

> - The output format doesn't look very machine-parseable. I really
> think we ought to provide something that is. Maybe a CSV-like format,
> or maybe something else, but I don't see why someone who wants to do
> change logging should be forced to write and install C code. If
> something like Bucardo can run on an unmodified system and extract
> change-sets this way without needing a .so file, that's going to be a
> huge win for usability.

We can change the current format but I really see little to no chance of
agreeing on a replication format that's serviceable to several solutions
short term. Once we've gained some experience - maybe even this cycle -
that might be different.

> More generally on this patch set, if I'm going to be committing any of
> this, I'd prefer to start with what is currently patches 3 and 4, once
> we reach agreement on those.

Sounds like a reasonable start.

> Are we hoping to get any of this committed for this CF? If so, let's
> make a plan to get that done; time is short. If not, let's update the
> CF app accordingly.

I'd really like to do so. I am travelling atm, but I will be back
tomorrow evening and will push an updated patch this weekend. The issue
I know of in the latest patches at
http://www.postgresql.org/message-id/20131007133232.GA15202@awork2.anarazel.de
is renaming from http://www.postgresql.org/message-id/20131008194758.GB3718183@alap2.anarazel.de

Do you know of anything else in the patches you're referring to?

Thanks,

Andres Freund

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

On Thu, Oct 10, 2013 at 7:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi Robert,
>
> On 2013-10-09 14:49:46 -0400, Robert Haas wrote:
>> I spent some time looking at the sample plugin (patch 9/12). Here are
>> some review comments:
>>
>> - I think that the decoding plugin interface should work more like the
>> foreign data wrapper interface. Instead of using pg_dlsym to look up
>> fixed names, I think there should be a struct of function pointers
>> that gets filled in and registered somehow.
>
> You mean something like CREATE OUTPUT PLUGIN registering a function with
> an INTERNAL return value returning a filled struct? I thought about
> that, but it seemed more complex. Happy to change it though if it's
> preferred.

I don't see any need for SQL syntax. I was just thinking that the
_PG_init function could fill in a structure and then call
RegisterLogicalReplicationOutputPlugin(&mystruct).

>> - Still wondering how we'll use this from a bgworker.
>
> Simplified code to consume data:

Cool. As long as that use case is supported, I'm happy; I just want
to make sure we're not presuming that there must be an external
client.

>> - The output format doesn't look very machine-parseable. I really
>> think we ought to provide something that is. Maybe a CSV-like format,
>> or maybe something else, but I don't see why someone who wants to do
>> change logging should be forced to write and install C code. If
>> something like Bucardo can run on an unmodified system and extract
>> change-sets this way without needing a .so file, that's going to be a
>> huge win for usability.
>
> We can change the current format but I really see little to no chance of
> agreeing on a replication format that's serviceable to several solutions
> short term. Once we've gained some experience - maybe even this cycle -
> that might be different.

I don't see why you're so pessimistic about that. I know you haven't
worked it out yet, but what makes this harder than sitting down and
designing something?

>> More generally on this patch set, if I'm going to be committing any of
>> this, I'd prefer to start with what is currently patches 3 and 4, once
>> we reach agreement on those.
>
> Sounds like a reasonable start.

Perhaps you could reshuffle the order of the series, if it's not too much work.

>> Are we hoping to get any of this committed for this CF? If so, let's
>> make a plan to get that done; time is short. If not, let's update the
>> CF app accordingly.
>
> I'd really like to do so. I am travelling atm, but I will be back
> tomorrow evening and will push an updated patch this weekend. The issue
> I know of in the latest patches at
> http://www.postgresql.org/message-id/20131007133232.GA15202@awork2.anarazel.de
> is renaming from http://www.postgresql.org/message-id/20131008194758.GB3718183@alap2.anarazel.de

I'm a bit nervous about the way the combo CID logging. I would have
thought that you would emit one record per combo CID, but what you're
apparently doing is emitting one record per heap tuple that uses a
combo CID. For some reason that feels like an abuse (and maybe kinda
inefficient, too).

Either way, I also wonder what happens if a (logical?) checkpoint
occurs between the combo CID record and the heap record to which it
refers, or how you prevent that from happening. What if the combo CID
record is written and the transaction aborts before writing the heap
record (maybe without writing an abort record to WAL)?

What are the performance implications of this additional WAL logging?
What's the worst case? What's the typical case? Does it have a
noticeable overhead when wal_level < logical?

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

Hi,

On 2013-10-11 09:08:43 -0400, Robert Haas wrote:
> On Thu, Oct 10, 2013 at 7:11 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-09 14:49:46 -0400, Robert Haas wrote:
> >> I spent some time looking at the sample plugin (patch 9/12). Here are
> >> some review comments:
> >>
> >> - I think that the decoding plugin interface should work more like the
> >> foreign data wrapper interface. Instead of using pg_dlsym to look up
> >> fixed names, I think there should be a struct of function pointers
> >> that gets filled in and registered somehow.
> >
> > You mean something like CREATE OUTPUT PLUGIN registering a function with
> > an INTERNAL return value returning a filled struct? I thought about
> > that, but it seemed more complex. Happy to change it though if it's
> > preferred.
>
> I don't see any need for SQL syntax. I was just thinking that the
> _PG_init function could fill in a structure and then call
> RegisterLogicalReplicationOutputPlugin(&mystruct).

Hm. We can do that, but what'd be the advantage of that? The current
model will correctly handle things like a'shared_preload_libraries'ed
output plugin, because its _PG_init() will not register it. With the
handling done in _PG_init() there would be two.
Being able to use the same .so for output plugin handling and some other
replication solution specific stuff is imo useful.

> >> - Still wondering how we'll use this from a bgworker.
> >
> > Simplified code to consume data:
>
> Cool. As long as that use case is supported, I'm happy; I just want
> to make sure we're not presuming that there must be an external
> client.

The included testcases are written using the SQL SRF interface, which in
turn is a usecase that doesn't use walsenders and such, so I hope we
won't break it accidentally ;)

> >> - The output format doesn't look very machine-parseable. I really
> >> think we ought to provide something that is. Maybe a CSV-like format,
> >> or maybe something else, but I don't see why someone who wants to do
> >> change logging should be forced to write and install C code. If
> >> something like Bucardo can run on an unmodified system and extract
> >> change-sets this way without needing a .so file, that's going to be a
> >> huge win for usability.
> >
> > We can change the current format but I really see little to no chance of
> > agreeing on a replication format that's serviceable to several solutions
> > short term. Once we've gained some experience - maybe even this cycle -
> > that might be different.
>
> I don't see why you're so pessimistic about that. I know you haven't
> worked it out yet, but what makes this harder than sitting down and
> designing something?

Because every replication solution has different requirements for the
format and they will want filter the output stream with regard to their
own configuration.
E.g. bucardo will want to include the transaction timestamp for conflict
resolution and such.

> >> More generally on this patch set, if I'm going to be committing any of
> >> this, I'd prefer to start with what is currently patches 3 and 4, once
> >> we reach agreement on those.
> >
> > Sounds like a reasonable start.
>
> Perhaps you could reshuffle the order of the series, if it's not too much work.

Sure, that's no problem. Do I understand correctly that you'd like
wal_decoding: Add information about a tables primary key to struct RelationData
wal_decoding: Add wal_level = logical and log data required for logical decoding

earlier?

> > I'd really like to do so. I am travelling atm, but I will be back
> > tomorrow evening and will push an updated patch this weekend. The issue
> > I know of in the latest patches at
> > http://www.postgresql.org/message-id/20131007133232.GA15202@awork2.anarazel.de
> > is renaming from http://www.postgresql.org/message-id/20131008194758.GB3718183@alap2.anarazel.de
>
> I'm a bit nervous about the way the combo CID logging. I would have
> thought that you would emit one record per combo CID, but what you're
> apparently doing is emitting one record per heap tuple that uses a
> combo CID.

I thought and implemented that in the beginning. Unfortunately it's not
enough :(. That's probably the issue that took me longest to understand
in this patchseries...

Combocids can only fix the case where a transaction actually has create
a combocid:

1) TX1: INSERT id = 1 at 0/1: (xmin = 1, xmax=Invalid, cmin = 55, cmax = Invalid)
2) TX2: DELETE id = 1 at 0/1: (xmin = 1, xmax=2, cmin = Invalid, cmax = 1)

So, if we're decoding data that needs to lookup those rows in TX1 or TX2
we both times need access to cmin and cmax, but neither transaction will
have created a multixact. That can only be an issue in transaction with
catalog modifications.

A slightly more complex variant also requires this if combocids are
involved:

1) TX1: INSERT id = 1 at 0/1: (xmin = 1, xmax=Invalid, cmin = 55, cmax = Invalid)
2) TX1: SAVEPOINT foo;
3) TX1-2: UPDATE id = 1 at 0/1: (xmin = 1, xmax=2, cmin = 55, cmax = 56, combo=123)
new at 0/1: (xmin = 2, xmax=Invalid, cmin = 57, cmax = Invalid)
4) TX1-2: ROLLBACK TO foo;
5) TX3: DELETE id = 1 at 0/1: (xmin = 1, xmax=3, cmin = Invalid, cmax = 1)

If you're decoding data that's been inserted after 1) you still need to
see cmin = 55. At 5) you need to see cmin = Invalid.

So just remembering the correct value for each tuple that's needed for a
specific transaction seems like the simplest way here.

> Either way, I also wonder what happens if a (logical?) checkpoint
> occurs between the combo CID record and the heap record to which it
> refers, or how you prevent that from happening.

Logical checkpoints contain the so called 'restart_decoding' LSN, which
is defined to be the LSN at which we can restart reading WAL and are
guaranteed to be able to decode all transactions that haven't been
confirmed as received.
Normal checkpoints shouldn't play any role here.

> What if the combo CID record is written and the transaction aborts
> before writing the heap record (maybe without writing an abort record
> to WAL)?

In the currently implemented model where we log (relfilenode, ctid,
cmin, cmax) we only ever need access to those rows when decoding data
changes from *within* the catalog modifying toplevel transaction. Never
in other toplevle transactions.

> What are the performance implications of this additional WAL logging?
> What's the worst case?

I haven't been able to notice any difference above jitter for anything
that touches actual relations. The overhead there is far bigger than
that single XLogInsert().
Maybe there's something that doesn't interact with much of the system
where the effort is noticeable and which is actually relvant for
performance? I couldn't really think of something.

> noticeable overhead when wal_level < logical?

Couldn't measure anything either, which is not surprising that I
couldn't measure the overhead in the first place.

I've done some parallel INSERT/DELETE pgbenching around the
wal_level=logical and I couldn't measure any overhead with it
disabled. With wal_level = logical, UPDATEs and DELETEs do get a bit
slower, but that's to be expected.

It'd probably not hurt to redo those benchmarks to make sure...

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 Fri, Oct 11, 2013 at 12:57 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I don't see any need for SQL syntax. I was just thinking that the
>> _PG_init function could fill in a structure and then call
>> RegisterLogicalReplicationOutputPlugin(&mystruct).
>
> Hm. We can do that, but what'd be the advantage of that? The current
> model will correctly handle things like a'shared_preload_libraries'ed
> output plugin, because its _PG_init() will not register it. With the
> handling done in _PG_init() there would be two.
> Being able to use the same .so for output plugin handling and some other
> replication solution specific stuff is imo useful.

Well, I just think relying on specific symbol names in the .so file is
kind of unfortunate. It means that, for example, you can't have
multiple output plugins provided by a single .so. And in general I
think it's something that we've tried to minimize.

>> I don't see why you're so pessimistic about that. I know you haven't
>> worked it out yet, but what makes this harder than sitting down and
>> designing something?
>
> Because every replication solution has different requirements for the
> format and they will want filter the output stream with regard to their
> own configuration.
> E.g. bucardo will want to include the transaction timestamp for conflict
> resolution and such.

But there's only so much information available here. Why not just
have a format that logs it all?

> Sure, that's no problem. Do I understand correctly that you'd like
> wal_decoding: Add information about a tables primary key to struct RelationData
> wal_decoding: Add wal_level = logical and log data required for logical decoding
>
> earlier?

Yes.

>> > I'd really like to do so. I am travelling atm, but I will be back
>> > tomorrow evening and will push an updated patch this weekend. The issue
>> > I know of in the latest patches at
>> > http://www.postgresql.org/message-id/20131007133232.GA15202@awork2.anarazel.de
>> > is renaming from http://www.postgresql.org/message-id/20131008194758.GB3718183@alap2.anarazel.de
>>
>> I'm a bit nervous about the way the combo CID logging. I would have
>> thought that you would emit one record per combo CID, but what you're
>> apparently doing is emitting one record per heap tuple that uses a
>> combo CID.
>
> I thought and implemented that in the beginning. Unfortunately it's not
> enough :(. That's probably the issue that took me longest to understand
> in this patchseries...
>
> Combocids can only fix the case where a transaction actually has create
> a combocid:
>
> 1) TX1: INSERT id = 1 at 0/1: (xmin = 1, xmax=Invalid, cmin = 55, cmax = Invalid)
> 2) TX2: DELETE id = 1 at 0/1: (xmin = 1, xmax=2, cmin = Invalid, cmax = 1)
>
> So, if we're decoding data that needs to lookup those rows in TX1 or TX2
> we both times need access to cmin and cmax, but neither transaction will
> have created a multixact. That can only be an issue in transaction with
> catalog modifications.

Oh, yuck. So that means you have to write an extra WAL record for
EVERY heap insert, update, or delete to a catalog table? OUCH.

> Couldn't measure anything either, which is not surprising that I
> couldn't measure the overhead in the first place.
>
> I've done some parallel INSERT/DELETE pgbenching around the
> wal_level=logical and I couldn't measure any overhead with it
> disabled. With wal_level = logical, UPDATEs and DELETEs do get a bit
> slower, but that's to be expected.
>
> 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.

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

On 2013-10-14 09:36:03 -0400, Robert Haas wrote:
> On Fri, Oct 11, 2013 at 12:57 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> I don't see any need for SQL syntax. I was just thinking that the
> >> _PG_init function could fill in a structure and then call
> >> RegisterLogicalReplicationOutputPlugin(&mystruct).
> >
> > Hm. We can do that, but what'd be the advantage of that? The current
> > model will correctly handle things like a'shared_preload_libraries'ed
> > output plugin, because its _PG_init() will not register it. With the
> > handling done in _PG_init() there would be two.
> > Being able to use the same .so for output plugin handling and some other
> > replication solution specific stuff is imo useful.
>
> Well, I just think relying on specific symbol names in the .so file is
> kind of unfortunate. It means that, for example, you can't have
> multiple output plugins provided by a single .so. And in general I
> think it's something that we've tried to minimize.

But that's not really different when you rely on _PG_init doing it's
thing, right?

> >> I don't see why you're so pessimistic about that. I know you haven't
> >> worked it out yet, but what makes this harder than sitting down and
> >> designing something?
> >
> > Because every replication solution has different requirements for the
> > format and they will want filter the output stream with regard to their
> > own configuration.
> > E.g. bucardo will want to include the transaction timestamp for conflict
> > resolution and such.
>
> But there's only so much information available here. Why not just
> have a format that logs it all?

Because we do not know what "all" is? Also, how would we handle
replication sets and such that all of the existing replication solutions
have generically?

> > Sure, that's no problem. Do I understand correctly that you'd like
> > wal_decoding: Add information about a tables primary key to struct RelationData
> > wal_decoding: Add wal_level = logical and log data required for logical decoding
> >
> > earlier?
>
> Yes.

That's done. Hope the new order makes sense.

> > So, if we're decoding data that needs to lookup those rows in TX1 or TX2
> > we both times need access to cmin and cmax, but neither transaction will
> > have created a multixact. That can only be an issue in transaction with
> > catalog modifications.

> Oh, yuck. So that means you have to write an extra WAL record for
> EVERY heap insert, update, or delete to a catalog table? OUCH.

Yes. We could integrate it into the main record without too many
problems, but it didn't seem like an important optimization and it would
have higher chances of slowing down wal_level < logical.

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

Greetings,

Andres Freund

--
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 Mon, Oct 14, 2013 at 9:51 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Well, I just think relying on specific symbol names in the .so file is
>> kind of unfortunate. It means that, for example, you can't have
>> multiple output plugins provided by a single .so. And in general I
>> think it's something that we've tried to minimize.
>
> But that's not really different when you rely on _PG_init doing it's
> thing, right?

Sure, that's true. But in general I think magic symbol names aren't a
particularly good design.

>> But there's only so much information available here. Why not just
>> have a format that logs it all?
>
> Because we do not know what "all" is? Also, how would we handle
> replication sets and such that all of the existing replication solutions
> have generically?

I don't see how you can fail to know what "all" is. There's only a
certain set of facts available. I mean you could log irrelevant crap
like a random number that you just picked or the sum of all numeric
values in the column, but nobody's likely to want that. What people
are going to want is the operation performed (insert, update, or
delete), all the values in the new tuple, the key values from the old
tuple, the transaction ID, and maybe some meta-information about the
transaction (such as the commit timestamp). What I'd probably do is
emit the data in CSV format, with the first column of each line being
a single character indicating what sort of row this is: H means a
header row, defining the format of subsequent rows
(H,table_name,new_column1,...,new_columnj,old_key_column1,...,old_key_columnk;
a new header row is emitted only when the column list changes); I, U,
or D means an insert, update, or delete, with column 2 being the
transaction ID, column 3 being the table name, and the remaining
columns matching the last header row for emitted for that table, T
means meta-information about a transaction, whatever we have (e.g.
T,txn_id,commit_time). There's probably some further tweaking of that
that could be done, and I might be overlooking some salient details,
like maybe we want to indicate the column types as well as their
names, but the range of things that someone can want to do here is not
unlimited. The point, for me anyway, is that someone can write a
crappy Perl script to apply changes from a file like this in a day.
My contention is that there are a lot of people who will want to do
just that, for one reason or another. The plugin interface has
awesome power and flexibility, and really high-performance replication
solutions will really benefit from that. But regular people don't
want to write C code; they just want to write a crappy Perl script.
And I think we can facilitate that without too much work.

>> Oh, yuck. So that means you have to write an extra WAL record for
>> EVERY heap insert, update, or delete to a catalog table? OUCH.
>
> Yes. We could integrate it into the main record without too many
> problems, but it didn't seem like an important optimization and it would
> have higher chances of slowing down wal_level < logical.

Hmm. I don't know whether that's an important optimization or not.

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

On Mon, Oct 14, 2013 at 5:07 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 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.

Any increase in WAL traffic will translate into a performance hit once
the I/O channel becomes saturated, but I agree those numbers don't
sound terrible for that faily-brutal test case. Actually, I was more
concerned about the hit on non-catalog workloads. pgbench isn't a
good test because the key column is so narrow; but suppose we have a
table like (a text, b integer, c text) where (a, c) is the primary key
and those strings are typically pretty long - say just short enough
that we can still index the column. It'd be worth testing both
workloads where the primary key doesn't change (so the only overhead
is figuring out that we need not log it) and those where it does
(where we're double-logging most of the tuple). I assume the latter
has to produce a significant hit to WAL volume, and I don't think
there's much we can do about that; but the former had better be nearly
free.

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

On 2013-10-15 08:42:20 -0400, Robert Haas wrote:
> On Mon, Oct 14, 2013 at 9:51 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> Well, I just think relying on specific symbol names in the .so file is
> >> kind of unfortunate. It means that, for example, you can't have
> >> multiple output plugins provided by a single .so. And in general I
> >> think it's something that we've tried to minimize.
> >
> > But that's not really different when you rely on _PG_init doing it's
> > thing, right?
>
> Sure, that's true. But in general I think magic symbol names aren't a
> particularly good design.

It allows you to use the shared libary both as a normal extension loaded
via shared_preload_library or adhoc and as an output plugin which seems
like a sensible goal.
We could have a single _PG_init_output_plugin() symbol that fills in
such a struct which would then not conflict with using the .so
independently. If you prefer that I'll change things around.

We can't do something like 'output_plugin_in_progress' before calling
_PG_init() because _PG_init() won't be called again if the shared object
is already loaded...

> >> But there's only so much information available here. Why not just
> >> have a format that logs it all?
> >
> > Because we do not know what "all" is? Also, how would we handle
> > replication sets and such that all of the existing replication solutions
> > have generically?
>
> I don't see how you can fail to know what "all" is. There's only a
> certain set of facts available. I mean you could log irrelevant crap
> like a random number that you just picked or the sum of all numeric
> values in the column, but nobody's likely to want that. What people
> are going to want is the operation performed (insert, update, or
> delete), all the values in the new tuple, the key values from the old
> tuple, the transaction ID, and maybe some meta-information about the
> transaction (such as the commit timestamp).

Some will want all column names included because that makes replication
into different schemas/databases easier, others won't because it makes
replicating the data more complicated and expensive.
Lots will want the primary key as a separate set of columns even for
inserts, others not.
There's also datatypes of values and null representation.

> What I'd probably do is
> emit the data in CSV format, with the first column of each line being
> a single character indicating what sort of row this is: H means a
> header row, defining the format of subsequent rows
> (H,table_name,new_column1,...,new_columnj,old_key_column1,...,old_key_columnk;
> a new header row is emitted only when the column list changes); I, U,
> or D means an insert, update, or delete, with column 2 being the
> transaction ID, column 3 being the table name, and the remaining
> columns matching the last header row for emitted for that table, T
> means meta-information about a transaction, whatever we have (e.g.
> T,txn_id,commit_time).

There's two issues I have with this:
a) CSV seems like a bad format for this. If a transaction inserts into
multiple tables the number of columns will constantly change. Many CSV
parsers don't deal with that all too gracefully. E.g. you can't even
load the data into another postgres database as an audit log.

If we go for CSV I think we should put the entire primary key as one
column (containing all the columns) and the entire row another.

We also don't have any nice facilities for actually writing CSV - so
we'll need to start extracting escaping code from COPY. In the end all
that will make the output plugin very hard to use as an example because
the code will get more complicated.

b) Emitting new row descriptors everytime the schema changes will
require keeping track of the schema. I think that won't be trivial. It
also makes consumption of the data more complicated in comparison to
including the description with every row.

Both are even more true once we extend the format to support streaming
of transactions while they are performed.

> But regular people don't want to write C code; they just want to write
> a crappy Perl script. And I think we can facilitate that without too
> much work.

I think the generic output plugin should be a separate one from the
example one (which is the one included in the patchset).

> >> Oh, yuck. So that means you have to write an extra WAL record for
> >> EVERY heap insert, update, or delete to a catalog table? OUCH.
> >
> > Yes. We could integrate it into the main record without too many
> > problems, but it didn't seem like an important optimization and it would
> > have higher chances of slowing down wal_level < logical.
>
> Hmm. I don't know whether that's an important optimization or not.

Based on the benchmark I'd say no. If we discover we need to go there we
can do so later. I don't forsee this to be really problematic.

Greetings,

Andres Freund

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

On 2013-10-15 08:49:26 -0400, Robert Haas wrote:
> On Mon, Oct 14, 2013 at 5:07 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > 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.
>
> Any increase in WAL traffic will translate into a performance hit once
> the I/O channel becomes saturated, but I agree those numbers don't
> sound terrible for that faily-brutal test case.

Well, the parallel workloads were fsync saturated although probably not
throughput, that's why I added them. But yes, it's not the same as a
throughput saturated IO channel.
Probably the worst case real-world workload is one that uses lots and
lots of ON COMMIT DROP temporary tables.

> Actually, I was more concerned about the hit on non-catalog workloads. pgbench isn't a
> good test because the key column is so narrow; but suppose we have a
> table like (a text, b integer, c text) where (a, c) is the primary key
> and those strings are typically pretty long - say just short enough
> that we can still index the column. It'd be worth testing both
> workloads where the primary key doesn't change (so the only overhead
> is figuring out that we need not log it) and those where it does
> (where we're double-logging most of the tuple). I assume the latter
> has to produce a significant hit to WAL volume, and I don't think
> there's much we can do about that; but the former had better be nearly
> free.

Ah, ok. Then I misunderstood you.

Is there a specific overhead you are "afraid" of in the
pkey-doesn't-change scenario? The changed wal logging (buffer in a
separate rdata entry) or the check whether the primary key has changed?

The only way I have been able to measure differences in that scenario
was to load a table with a low fillfactor and wide tuples, checkpoint,
and then update lots of rows. On wal_level=logical that will result in
full-page-images and tuple data being logged which can be noticeable if
you have really large tuples, even if the pkey doesn't change.

We could optimize that by not actually logging the tuple data in that
case but just include the tid so we could extract things from the Bkp
block ourselves. But that will complicate the code and doesn't yet seem
warranted.

Greetings,

Andres Freund

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

On 2013-10-15 15:17:58 +0200, Andres Freund wrote:
> If we go for CSV I think we should put the entire primary key as one
> column (containing all the columns) and the entire row another.

What about columns like:
* action B|I|U|D|C

* xid
* timestamp

* tablename

* key name
* key column names
* key column types

* new key column values

* column names
* column types
* column values

* candidate_key_changed?
* old key column values

And have output plugin options
* include-column-types
* include-column-names
* include-primary-key

If something isn't included it's simply left out.

What still need to be determined is:
* how do we separate and escape multiple values in one CSV column
* how do we represent NULLs

Greetings,

Andres Freund

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

On Tue, Oct 15, 2013 at 9:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> It allows you to use the shared libary both as a normal extension loaded
> via shared_preload_library or adhoc and as an output plugin which seems
> like a sensible goal.
> We could have a single _PG_init_output_plugin() symbol that fills in
> such a struct which would then not conflict with using the .so
> independently. If you prefer that I'll change things around.

I think part of the problem may be that you're using the library name
to identify the output plugin. I'm not excited about that design.
For functions, you give the function a name and that is a pointer to
where to actually find the function, which may be a 2-tuple
<library-name, function-name>, or perhaps just a 1-tuple
<builtin-function-name>, or maybe the whole text of a PL/pgsql
procedure that should be compiled.

Perhaps this ought to work similarly. Create a function in pg_proc
which returns the structure containing the function pointers. Then,
when that output plugin is selected, it'll automatically trigger
loading the correct shared library if that's needed; and the shared
library name may (but need not) match the output plugin name.

>> What I'd probably do is
>> emit the data in CSV format, with the first column of each line being
>> a single character indicating what sort of row this is: H means a
>> header row, defining the format of subsequent rows
>> (H,table_name,new_column1,...,new_columnj,old_key_column1,...,old_key_columnk;
>> a new header row is emitted only when the column list changes); I, U,
>> or D means an insert, update, or delete, with column 2 being the
>> transaction ID, column 3 being the table name, and the remaining
>> columns matching the last header row for emitted for that table, T
>> means meta-information about a transaction, whatever we have (e.g.
>> T,txn_id,commit_time).
>
> There's two issues I have with this:
> a) CSV seems like a bad format for this. If a transaction inserts into
> multiple tables the number of columns will constantly change. Many CSV
> parsers don't deal with that all too gracefully. E.g. you can't even
> load the data into another postgres database as an audit log.

We can pick some other separator. I don't think ragged CSV is a big
problem; I'm actually more worried about having an easy way to handle
embedded commas and newlines and so on. But I'd be fine with
tab-separated data or something too, if you think that's better. What
I want is something that someone can parse with a script that can be
written in a reasonable amount of time in their favorite scripting
language. I predict that if we provide something like this we'll
vastly expand the number of users who can make use of this new
functionality.

User: So, what's new in PostgreSQL 9.4?
Hacker: Well, now we have logical replication!
User: Why is that cool?
Hacker: Well, streaming replication is awesome for HA, but it has
significant limitations. And trigger-based systems are very mature,
but the overhead is high and their lack of core integration makes them
hard to use. With this technology, you can build systems that will
replicate individual tables or even parts of tables, multi-master
systems, and lots of other cool stuff.
User: Wow, that sounds great. How do I use it?
Hacker: Well, first you write an output plugin in C using a special API.
User: Hey, do you know whether the MongoDB guys came to this conference?

Let's try that again.

User: Wow, that sounds great. How do I use it?
Hacker: Well, currently, the output gets dumped as a series of text
files that are designed to be parsed using a scripting language. We
have sample parsers written in Perl and Python that you can use as-is
or hack up to meet your needs.

Now, some users are still going to head for the hills. But at least
from where I sit it sounds a hell of a lot better than the first
answer. We're not going to solve all of the tooling problems around
this technology in one release, for sure. But as far as 95% of our
users are concerned, a C API might as well not exist at all. People
WILL try to machine parse the output of whatever demo plugins we
provide; so I think we should try hard to provide at least one such
plugin that is designed to make that as easy as possible.

> If we go for CSV I think we should put the entire primary key as one
> column (containing all the columns) and the entire row another.
>
> We also don't have any nice facilities for actually writing CSV - so
> we'll need to start extracting escaping code from COPY. In the end all
> that will make the output plugin very hard to use as an example because
> the code will get more complicated.
>
> b) Emitting new row descriptors everytime the schema changes will
> require keeping track of the schema. I think that won't be trivial. It
> also makes consumption of the data more complicated in comparison to
> including the description with every row.
>
> Both are even more true once we extend the format to support streaming
> of transactions while they are performed.

All fair points, but IMHO this is exactly why we need to provide a
well-written output plugin, not leave it to users to solve these
problems.

>> But regular people don't want to write C code; they just want to write
>> a crappy Perl script. And I think we can facilitate that without too
>> much work.
>
> I think the generic output plugin should be a separate one from the
> example one (which is the one included in the patchset).

That's OK with me.

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

On 10/15/2013 01:42 PM, Robert Haas wrote:
> On Mon, Oct 14, 2013 at 9:51 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> Well, I just think relying on specific symbol names in the .so file is
>>> kind of unfortunate. It means that, for example, you can't have
>>> multiple output plugins provided by a single .so. And in general I
>>> think it's something that we've tried to minimize.
>> But that's not really different when you rely on _PG_init doing it's
>> thing, right?
> Sure, that's true. But in general I think magic symbol names aren't a
> particularly good design.
>
>>> But there's only so much information available here. Why not just
>>> have a format that logs it all?
>> Because we do not know what "all" is? Also, how would we handle
>> replication sets and such that all of the existing replication solutions
>> have generically?
> I don't see how you can fail to know what "all" is.
We instinctively know what "all" is - as in the famous case of buddhist
ordering a
hamburger - "Make me All wit Everything" :) - but the requirements of
different replications systems vary wildly.

> ...
> What people
> are going to want is the operation performed (insert, update, or
> delete), all the values in the new tuple, the key values from the old
> tuple,
For multi-master / conflict resolution you may also want all old
values to make sure that they have not changed on target.

the difference in WAL volume can be really significant, especially
in the case of DELETE, where there are no new columns.

for some forms of conflict resolution we may even want to know
the database user who initiated the operation. and possibly even
some session variables like "very_important=yes".

> ... The point, for me anyway, is that someone can write a
> crappy Perl script to apply changes from a file like this in a day.
> My contention is that there are a lot of people who will want to do
> just that, for one reason or another. The plugin interface has
> awesome power and flexibility, and really high-performance replication
> solutions will really benefit from that. But regular people don't
> want to write C code; they just want to write a crappy Perl script.
> And I think we can facilitate that without too much work.
just provide a to-csv or to-json plugin and the crappy perl guys
are happy.

On 10/15/2013 02:47 PM, Andres Freund wrote:
> On 2013-10-15 15:17:58 +0200, Andres Freund wrote:
>> If we go for CSV I think we should put the entire primary key as one
>> column (containing all the columns) and the entire row another.
just use JSON :)
>> What about columns like:
>> * action B|I|U|D|C
>>
>> * xid
>> * timestamp
>>
>> * tablename
>>
>> * key name
>> * key column names
>> * key column types
>>
>> * new key column values
>>
>> * column names
>> * column types
>> * column values
>>
>> * candidate_key_changed?
>> * old key column values
>>
>> And have output plugin options
>> * include-column-types
>> * include-column-names
>> * include-primary-key
>>
>> If something isn't included it's simply left out.
>>
>> What still need to be determined is:
>> * how do we separate and escape multiple values in one CSV column
>> * how do we represent NULLs

or borrow whatever possible from pg_dump as they have
needed to solve most of the same problems already and
consistency is good in general
>
> Greetings,
>
> Andres Freund
>

On Tue, Oct 15, 2013 at 9:47 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2013-10-15 15:17:58 +0200, Andres Freund wrote:
>> If we go for CSV I think we should put the entire primary key as one
>> column (containing all the columns) and the entire row another.
>
> What about columns like:
> * action B|I|U|D|C

BEGIN and COMMIT?

> * xid
> * timestamp
>
> * tablename
>
> * key name
> * key column names
> * key column types
>
> * new key column values
>
> * column names
> * column types
> * column values
>
> * candidate_key_changed?
> * old key column values

Repeating the column names for every row strikes me as a nonstarter.
If the plugin interface isn't rich enough to provide a convenient way
to avoid that, then it needs to be fixed so that it is, because it
will be a common requirement. Sure, some people may want JSON or XML
output that reiterates the labels every time, but for a lot of people
that's going to greatly increase the size of the output and be
undesirable for that reason.

> What still need to be determined is:
> * how do we separate and escape multiple values in one CSV column
> * how do we represent NULLs

I consider the escaping a key design decision. Ideally, it should be
something that's easy to reverse from a scripting language; ideally
also, it should be something similar to how we handle COPY. These
goals may be in conflict; we'll have to pick something.

I'm not sure that having multiple values in one column is a good plan,
because now you need multiple levels of parsing to unpack the row.
I'd rather just have a flat column list with a key somewhere
explaining how to interpret the data. But I'm prepared to give in on
that point so long as we can demonstrate that the format can be easily
parsed.

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

On Tue, Oct 15, 2013 at 10:09 AM, Hannu Krosing <hannu(at)krosing(dot)net> wrote:
>> I don't see how you can fail to know what "all" is.
> We instinctively know what "all" is - as in the famous case of buddhist
> ordering a
> hamburger - "Make me All wit Everything" :) - but the requirements of
> different replications systems vary wildly.

That's true to some degree, but let's not exaggerate the degree to
which it is true.

> For multi-master / conflict resolution you may also want all old
> values to make sure that they have not changed on target.

The patch as proposed doesn't make that information available. If you
want that to be an option, now would be the right time to argue for
it.

> for some forms of conflict resolution we may even want to know
> the database user who initiated the operation. and possibly even
> some session variables like "very_important=yes".

Well, if you have requirements like logging very_important=yes, then
you're definitely into the territory where you need your own output
plugin. I have no problem telling people who want that sort of thing
that they've got to go write C code. What I'm trying to do, as Larry
Wall once said, is to make simple things simple and hard things
possible. The output plugin interface accomplishes the latter, but,
by itself, not the former.

>> And I think we can facilitate that without too much work.
> just provide a to-csv or to-json plugin and the crappy perl guys
> are happy.

Yep, that's exactly what I'm advocating for.

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

On 2013-10-15 10:20:55 -0400, Robert Haas wrote:
> > For multi-master / conflict resolution you may also want all old
> > values to make sure that they have not changed on target.
>
> The patch as proposed doesn't make that information available. If you
> want that to be an option, now would be the right time to argue for
> it.

I don't think you necessarily want it for most MM solutions, but I agree
it will be useful for some scenarios.

I think the ReorderBufferChange struct needs a better way to distinguish
between old-key and old-tuple now, but I'd rather implement the
facililty for logging the full old tuple in a separate patch. The
patchset is big enough as is, lets not tack on more features.

Greetings,

Andres Freund

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

On 2013-10-15 10:09:05 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 9:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > It allows you to use the shared libary both as a normal extension loaded
> > via shared_preload_library or adhoc and as an output plugin which seems
> > like a sensible goal.
> > We could have a single _PG_init_output_plugin() symbol that fills in
> > such a struct which would then not conflict with using the .so
> > independently. If you prefer that I'll change things around.
>
> I think part of the problem may be that you're using the library name
> to identify the output plugin. I'm not excited about that design.
> For functions, you give the function a name and that is a pointer to
> where to actually find the function, which may be a 2-tuple
> <library-name, function-name>, or perhaps just a 1-tuple
> <builtin-function-name>, or maybe the whole text of a PL/pgsql
> procedure that should be compiled.

That means you allow trivial remote code execution since you could try
to load system() or something else that's available in every shared
object. Now you can argue that that's OK since we have special checks
for replication connections, but I'd rather not go there.

> Perhaps this ought to work similarly. Create a function in pg_proc
> which returns the structure containing the function pointers. Then,
> when that output plugin is selected, it'll automatically trigger
> loading the correct shared library if that's needed; and the shared
> library name may (but need not) match the output plugin name.

I'd like to avoid relying on inserting stuff into pg_proc because that
makes it harder to extract WAL from a HS standby. Requiring to configure
that on the primary to extract data on the standby seems confusing to
me.

But perhaps that's the correct solution :/

> Now, some users are still going to head for the hills. But at least
> from where I sit it sounds a hell of a lot better than the first
> answer. We're not going to solve all of the tooling problems around
> this technology in one release, for sure. But as far as 95% of our
> users are concerned, a C API might as well not exist at all. People
> WILL try to machine parse the output of whatever demo plugins we
> provide; so I think we should try hard to provide at least one such
> plugin that is designed to make that as easy as possible.

Well, just providing the C API + an example in a first step didn't work
out too badly for FDWs. I am pretty sure that once released there will
soon be extensions for it on PGXN or whatever for special usecases.

Greetings,

Andres Freund

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

On Tue, Oct 15, 2013 at 10:27 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I think part of the problem may be that you're using the library name
>> to identify the output plugin. I'm not excited about that design.
>> For functions, you give the function a name and that is a pointer to
>> where to actually find the function, which may be a 2-tuple
>> <library-name, function-name>, or perhaps just a 1-tuple
>> <builtin-function-name>, or maybe the whole text of a PL/pgsql
>> procedure that should be compiled.
>
> That means you allow trivial remote code execution since you could try
> to load system() or something else that's available in every shared
> object. Now you can argue that that's OK since we have special checks
> for replication connections, but I'd rather not go there.

Well, obviously you can't let somebody load any library they want.
But that's pretty much true anyway; LOAD had better be confined to
superusers unless there is something (like a pg_proc entry) that
provides "prior authorization" for that specific load.

>> Perhaps this ought to work similarly. Create a function in pg_proc
>> which returns the structure containing the function pointers. Then,
>> when that output plugin is selected, it'll automatically trigger
>> loading the correct shared library if that's needed; and the shared
>> library name may (but need not) match the output plugin name.
>
> I'd like to avoid relying on inserting stuff into pg_proc because that
> makes it harder to extract WAL from a HS standby. Requiring to configure
> that on the primary to extract data on the standby seems confusing to
> me.
>
> But perhaps that's the correct solution :/

That's a reasonable concern. I don't have another idea at the moment,
unless we want to allow replication connections to issue LOAD
commands. Then you can LOAD the library, so that the plug-in is
registered under the well-known name you expect it to have, and then
use that name to start replication.

>> Now, some users are still going to head for the hills. But at least
>> from where I sit it sounds a hell of a lot better than the first
>> answer. We're not going to solve all of the tooling problems around
>> this technology in one release, for sure. But as far as 95% of our
>> users are concerned, a C API might as well not exist at all. People
>> WILL try to machine parse the output of whatever demo plugins we
>> provide; so I think we should try hard to provide at least one such
>> plugin that is designed to make that as easy as possible.
>
> Well, just providing the C API + an example in a first step didn't work
> out too badly for FDWs. I am pretty sure that once released there will
> soon be extensions for it on PGXN or whatever for special usecases.

I suspect so, too. But I also think that if that's the only thing
available in the first release, a lot of users will get a poor initial
impression.

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

On 2013-10-15 10:15:14 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 9:47 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2013-10-15 15:17:58 +0200, Andres Freund wrote:
> >> If we go for CSV I think we should put the entire primary key as one
> >> column (containing all the columns) and the entire row another.
> >
> > What about columns like:
> > * action B|I|U|D|C
>
> BEGIN and COMMIT?

That's B and C, yes. You'd rather not have them? When would you replay
the commit without an explicit message telling you to?

> Repeating the column names for every row strikes me as a nonstarter.
> [...]
> Sure, some people may want JSON or XML
> output that reiterates the labels every time, but for a lot of people
> that's going to greatly increase the size of the output and be
> undesirable for that reason.

But I argue that most simpler users - which are exactly the ones a
generic output plugin is aimed at - will want all column names since it
makes replay far easier.

> If the plugin interface isn't rich enough to provide a convenient way
> to avoid that, then it needs to be fixed so that it is, because it
> will be a common requirement.

Oh, it surely is possibly to avoid repeating it. The output plugin
interface simply gives you a relcache entry, that contains everything
necessary.
The output plugin would need to keep track of whether it has output data
for a specific relation and it would need to check whether the table
definition has changed, but I don't see how we could avoid that?

> > What still need to be determined is:
> > * how do we separate and escape multiple values in one CSV column
> > * how do we represent NULLs
>
> I consider the escaping a key design decision. Ideally, it should be
> something that's easy to reverse from a scripting language; ideally
> also, it should be something similar to how we handle COPY. These
> goals may be in conflict; we'll have to pick something.

Note that parsing COPYs is a major PITA from most languages...

Perhaps we should make the default output json instead? With every
action terminated by a nullbyte?
That's probably easier to parse from various scripting languages than
anything else.

Greetings,

Andres Freund

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

On 2013-10-15 10:34:53 -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 10:27 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> I think part of the problem may be that you're using the library name
> >> to identify the output plugin. I'm not excited about that design.
> >> For functions, you give the function a name and that is a pointer to
> >> where to actually find the function, which may be a 2-tuple
> >> <library-name, function-name>, or perhaps just a 1-tuple
> >> <builtin-function-name>, or maybe the whole text of a PL/pgsql
> >> procedure that should be compiled.
> >
> > That means you allow trivial remote code execution since you could try
> > to load system() or something else that's available in every shared
> > object. Now you can argue that that's OK since we have special checks
> > for replication connections, but I'd rather not go there.
>
> Well, obviously you can't let somebody load any library they want.
> But that's pretty much true anyway; LOAD had better be confined to
> superusers unless there is something (like a pg_proc entry) that
> provides "prior authorization" for that specific load.

Currently you can create users that have permissions for replication but
which are not superusers. I think we should strive to providing that
capability for changeset extraction as well.

> >> Perhaps this ought to work similarly. Create a function in pg_proc
> >> which returns the structure containing the function pointers. Then,
> >> when that output plugin is selected, it'll automatically trigger
> >> loading the correct shared library if that's needed; and the shared
> >> library name may (but need not) match the output plugin name.
> >
> > I'd like to avoid relying on inserting stuff into pg_proc because that
> > makes it harder to extract WAL from a HS standby. Requiring to configure
> > that on the primary to extract data on the standby seems confusing to
> > me.
> >
> > But perhaps that's the correct solution :/
>
> That's a reasonable concern. I don't have another idea at the moment,
> unless we want to allow replication connections to issue LOAD
> commands. Then you can LOAD the library, so that the plug-in is
> registered under the well-known name you expect it to have, and then
> use that name to start replication.

But what's the advantage of that over the current situation or one where
PG_load_output_plugin() is called? The current and related
implementations allow you to only load libraries in some designated
postgres directories and it doesn't allow you to call any arbitrary
functions in there.

Would you be content with a symbol "PG_load_output_plugin" being called
that fills out the actual callbacks?

> >> Now, some users are still going to head for the hills. But at least
> >> from where I sit it sounds a hell of a lot better than the first
> >> answer. We're not going to solve all of the tooling problems around
> >> this technology in one release, for sure. But as far as 95% of our
> >> users are concerned, a C API might as well not exist at all. People
> >> WILL try to machine parse the output of whatever demo plugins we
> >> provide; so I think we should try hard to provide at least one such
> >> plugin that is designed to make that as easy as possible.
> >
> > Well, just providing the C API + an example in a first step didn't work
> > out too badly for FDWs. I am pretty sure that once released there will
> > soon be extensions for it on PGXN or whatever for special usecases.
>
> I suspect so, too. But I also think that if that's the only thing
> available in the first release, a lot of users will get a poor initial
> impression.

I think lots of people will expect a builtin logical replication
solution :/. Which seems a tad unlikely to arrive in 9.4.

Greetings,

Andres Freund

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

On Tue, Oct 15, 2013 at 10:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > What about columns like:
>> > * action B|I|U|D|C
>>
>> BEGIN and COMMIT?
>
> That's B and C, yes. You'd rather not have them? When would you replay
> the commit without an explicit message telling you to?

No, BEGIN and COMMIT sounds good, actually. Just wanted to make sure
I understood.

>> Repeating the column names for every row strikes me as a nonstarter.
>> [...]
>> Sure, some people may want JSON or XML
>> output that reiterates the labels every time, but for a lot of people
>> that's going to greatly increase the size of the output and be
>> undesirable for that reason.
>
> But I argue that most simpler users - which are exactly the ones a
> generic output plugin is aimed at - will want all column names since it
> makes replay far easier.

Meh, maybe.

>> If the plugin interface isn't rich enough to provide a convenient way
>> to avoid that, then it needs to be fixed so that it is, because it
>> will be a common requirement.
>
> Oh, it surely is possibly to avoid repeating it. The output plugin
> interface simply gives you a relcache entry, that contains everything
> necessary.
> The output plugin would need to keep track of whether it has output data
> for a specific relation and it would need to check whether the table
> definition has changed, but I don't see how we could avoid that?

Well, it might be nice if there were a callback for, hey, schema has
changed! Seems like a lot of plugins will want to know that for one
reason or another, and rechecking for every tuple sounds expensive.

>> > What still need to be determined is:
>> > * how do we separate and escape multiple values in one CSV column
>> > * how do we represent NULLs
>>
>> I consider the escaping a key design decision. Ideally, it should be
>> something that's easy to reverse from a scripting language; ideally
>> also, it should be something similar to how we handle COPY. These
>> goals may be in conflict; we'll have to pick something.
>
> Note that parsing COPYs is a major PITA from most languages...
>
> Perhaps we should make the default output json instead? With every
> action terminated by a nullbyte?
> That's probably easier to parse from various scripting languages than
> anything else.

I could go for that. It's not quite as compact as I might hope, but
JSON does seem to make people awfully happy.

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

On Tue, Oct 15, 2013 at 10:56 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > That means you allow trivial remote code execution since you could try
>> > to load system() or something else that's available in every shared
>> > object. Now you can argue that that's OK since we have special checks
>> > for replication connections, but I'd rather not go there.
>>
>> Well, obviously you can't let somebody load any library they want.
>> But that's pretty much true anyway; LOAD had better be confined to
>> superusers unless there is something (like a pg_proc entry) that
>> provides "prior authorization" for that specific load.
>
> Currently you can create users that have permissions for replication but
> which are not superusers. I think we should strive to providing that
> capability for changeset extraction as well.

I agree.

>> >> Perhaps this ought to work similarly. Create a function in pg_proc
>> >> which returns the structure containing the function pointers. Then,
>> >> when that output plugin is selected, it'll automatically trigger
>> >> loading the correct shared library if that's needed; and the shared
>> >> library name may (but need not) match the output plugin name.
>> >
>> > I'd like to avoid relying on inserting stuff into pg_proc because that
>> > makes it harder to extract WAL from a HS standby. Requiring to configure
>> > that on the primary to extract data on the standby seems confusing to
>> > me.
>> >
>> > But perhaps that's the correct solution :/
>>
>> That's a reasonable concern. I don't have another idea at the moment,
>> unless we want to allow replication connections to issue LOAD
>> commands. Then you can LOAD the library, so that the plug-in is
>> registered under the well-known name you expect it to have, and then
>> use that name to start replication.
>
> But what's the advantage of that over the current situation or one where
> PG_load_output_plugin() is called? The current and related
> implementations allow you to only load libraries in some designated
> postgres directories and it doesn't allow you to call any arbitrary
> functions in there.

Well, I've already said why I don't like conflating the library name
and the plugin name. It rules out core plugins and libraries that
provide multiple plugins. I don't have anything to add to that.

> Would you be content with a symbol "PG_load_output_plugin" being called
> that fills out the actual callbacks?

Well, it doesn't fix the muddling of library names with output plugin
names, but I suppose I'd find it a modest improvement.

>> > Well, just providing the C API + an example in a first step didn't work
>> > out too badly for FDWs. I am pretty sure that once released there will
>> > soon be extensions for it on PGXN or whatever for special usecases.
>>
>> I suspect so, too. But I also think that if that's the only thing
>> available in the first release, a lot of users will get a poor initial
>> impression.
>
> I think lots of people will expect a builtin logical replication
> solution :/. Which seems a tad unlikely to arrive in 9.4.

Yep.

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

On Tue, Oct 15, 2013 at 11:02:39AM -0400, Robert Haas wrote:
> >> goals may be in conflict; we'll have to pick something.
> >
> > Note that parsing COPYs is a major PITA from most languages...
> >
> > Perhaps we should make the default output json instead? With every
> > action terminated by a nullbyte?
> > That's probably easier to parse from various scripting languages than
> > anything else.
>
> I could go for that. It's not quite as compact as I might hope, but
> JSON does seem to make people awfully happy.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>

Feeding such a JSON stream into a compression algorithm like lz4 or
snappy should result in a pretty compact stream. The latest lz4 updates
also have ability to use a pre-existing dictionary which would really
help remove the redundant pieces.

Regards,
Ken

On 2013-10-15 11:02:39 -0400, Robert Haas wrote:
> >> If the plugin interface isn't rich enough to provide a convenient way
> >> to avoid that, then it needs to be fixed so that it is, because it
> >> will be a common requirement.
> >
> > Oh, it surely is possibly to avoid repeating it. The output plugin
> > interface simply gives you a relcache entry, that contains everything
> > necessary.
> > The output plugin would need to keep track of whether it has output data
> > for a specific relation and it would need to check whether the table
> > definition has changed, but I don't see how we could avoid that?
>
> Well, it might be nice if there were a callback for, hey, schema has
> changed! Seems like a lot of plugins will want to know that for one
> reason or another, and rechecking for every tuple sounds expensive.

I don't really see how we could provide that in any useful manner. We
could provide a callback that is called whenever another transaction has
changed the schema, but there's nothing easily to be done about schema
changes by the replayed transaction itself. And those are the only ones
where meaningful schema changes can happen since the locks the source
transaction has held will prevent most other schema changes.

As much as I hate such code, I guess checking (and possibly storing) the
ctid||xmin of the pg_class row is the easiest thing we could do :(.

Greetings,

Andres Freund

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

On 10/15/2013 07:56 AM, Andres Freund wrote:
>>> Well, just providing the C API + an example in a first step didn't work
>>> > > out too badly for FDWs. I am pretty sure that once released there will
>>> > > soon be extensions for it on PGXN or whatever for special usecases.
>> >
>> > I suspect so, too. But I also think that if that's the only thing
>> > available in the first release, a lot of users will get a poor initial
>> > impression.
> I think lots of people will expect a builtin logical replication
> solution :/. Which seems a tad unlikely to arrive in 9.4.

Well, last I checked the Slony team is hard at work on building
something which will be based on logical changesets. So there will
likely be at least one tool available shortly after 9.4 is released.

A good and flexible API is, IMHO, more important than having any
finished solution. The whole reason why logical replication was outside
core PG for so long is that replication systems have differing and
mutually incompatible goals. A good API can support all of those goals;
a user-level tool, no matter how good, can't.

And, frankly, once the API is built, how hard will it be to write a
script which does the simplest replication approach (replay all
statements on slave)?

--
Josh Berkus
PostgreSQL Experts Inc.
http://pgexperts.com

On Tue, Oct 15, 2013 at 10:09:05AM -0400, Robert Haas wrote:
> On Tue, Oct 15, 2013 at 9:17 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> User: So, what's new in PostgreSQL 9.4?
> Hacker: Well, now we have logical replication!
> User: Why is that cool?
> Hacker: Well, streaming replication is awesome for HA, but it has
> significant limitations. And trigger-based systems are very mature,
> but the overhead is high and their lack of core integration makes them
> hard to use. With this technology, you can build systems that will
> replicate individual tables or even parts of tables, multi-master
> systems, and lots of other cool stuff.
> User: Wow, that sounds great. How do I use it?
> Hacker: Well, first you write an output plugin in C using a special API.
> User: Hey, do you know whether the MongoDB guys came to this conference?
>
> Let's try that again.
>
> User: Wow, that sounds great. How do I use it?
> Hacker: Well, currently, the output gets dumped as a series of text
> files that are designed to be parsed using a scripting language. We
> have sample parsers written in Perl and Python that you can use as-is
> or hack up to meet your needs.

My version:

Hacker: the output gets dumped as a series of JSON files. We have
docs for this rev of the format and examples of consumers in Perl and
Python you can use as-is or hack up to meet your needs.

Cheers,
David.
--
David Fetter <david(at)fetter(dot)org> http://fetter.org/
Phone: +1 415 235 3778 AIM: dfetter666 Yahoo!: dfetter
Skype: davidfetter XMPP: david(dot)fetter(at)gmail(dot)com
iCal: webcal://www.tripit.com/feed/ical/people/david74/tripit.ics

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

On Tue, Oct 15, 2013 at 7:09 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> Let's try that again.
>
> User: Wow, that sounds great. How do I use it?
> Hacker: Well, currently, the output gets dumped as a series of text
> files that are designed to be parsed using a scripting language. We
> have sample parsers written in Perl and Python that you can use as-is
> or hack up to meet your needs.

Have you heard of multicorn? Plugin authors can write a wrapper that
spits out JSON or whatever other thing they like, which can be
consumed by non C-hackers.

> Now, some users are still going to head for the hills. But at least
> from where I sit it sounds a hell of a lot better than the first
> answer. We're not going to solve all of the tooling problems around
> this technology in one release, for sure. But as far as 95% of our
> users are concerned, a C API might as well not exist at all. People
> WILL try to machine parse the output of whatever demo plugins we
> provide; so I think we should try hard to provide at least one such
> plugin that is designed to make that as easy as possible.

I agree that this is important, but I wouldn't like to weigh it too heavily.

--
Peter Geoghegan

On Mon, Oct 14, 2013 at 9:12 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Attached you can find version 6.4 of the patchset:

So I'm still unhappy with the arbitrary logic in what's now patch 1
for choosing the candidate key. On another thread, someone mentioned
that they might want the entire old tuple, and that got me thinking:
there's no particular reason why the user has to want exactly the
columns that exist in some unique, immediate, non-partial index (what
a name). So I have two proposals:

1. Instead of allowing the user to choose the index to be used, or
picking it for them, how about if we let them choose the old-tuple
columns they want logged? This could be a per-column option. If the
primary key can be assumed known and unchanging, then the answer might
be that the user wants *no* old-tuple columns logged. Contrariwise
someone might want everything logged, or anything in the middle.

2. If that seems too complicated, how about just logging the whole old
tuple for version 1?

I'm basically fine with the rest of what's in the first two patches,
but we need to sort out some kind of consensus on this issue.

Thanks,

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