logical changeset generation v6

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

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

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

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

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

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

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

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

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

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

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@2ndquadrant.com> wrote:

On 2013-09-17 09:45:28 -0400, Peter Eisentraut wrote:

On 9/15/13 11:30 AM, Andres Freund wrote:

On 2013-09-15 11:20:20 -0400, Peter Eisentraut wrote:

On Sat, 2013-09-14 at 22:49 +0200, Andres Freund wrote:

Attached you can find the newest version of the logical changeset
generation patchset.

You probably have bigger things to worry about, but please check the
results of cpluspluscheck, because some of the header files don't
include header files they depend on.

Hm. I tried to get that right, but it's been a while since I last
checked. I don't regularly use cpluspluscheck because it doesn't work in
VPATH builds... We really need to fix that.

I'll push a fix for that to the git tree, don't think that's worth a
resend in itself.

This patch set now fails to apply because of the commit "Rename various
"freeze multixact" variables".

And I am even partially guilty for that patch...

Rebased patches attached.

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

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

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

LOG: server process (PID 12777) was terminated by signal 11:
Segmentation fault

Regards,

--
Fujii Masao

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

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@2ndquadrant.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

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

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@2ndquadrant.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

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

On Thu, Sep 19, 2013 at 10:43 AM, Andres Freund <andres@2ndquadrant.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

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

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

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

On Thu, Sep 19, 2013 at 7:43 AM, Andres Freund <andres@2ndquadrant.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

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

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

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

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

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

On Mon, Sep 23, 2013 at 1:46 AM, Andres Freund <andres@2ndquadrant.com> wrote:

pg_receivelogical? Protest now or forever hold your peace.

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

--
Peter Geoghegan

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

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

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

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

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

On Mon, Sep 23, 2013 at 9:54 AM, Andres Freund <andres@2ndquadrant.com> wrote:

I still find it wierd/inconsistent to have:
* pg_receivexlog
* pg_recvlogical
binaries, even from the same source directory. Why once "pg_recv" and
once "pg_receive"?

+1

--
Peter Geoghegan

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