Re: Changeset Extraction v7.6.1

Hello Everyone,

I am pleased to announce the next version of the the changeset
extraction feature.

There's more changes than I can remember, but that's what comes to my
tired mind:
* Initial userlevel docs (including an example session!).
* generalization of the "replication slot" system to also work for
streaming rep, although the user interface for that is mostly missing.
* don't remove WAL still required by a replication slot, be it a slot for
changeset extraction or streaming rep.
* New output plugin interface with one _PG_output_plugin_init dlsym()ed
function filling out the callbacks.
* Renaming of the init and _cleanup output plugins to startup and shutdown.
* Simplification of the prepare_write/write interface for output plugins
(no need to specify LSNs anymore).
* Renaming of the changeset extraction operations to
create_replication_slot/drop_replication_slot/start_replication
... logical.
* moving the SQL changeset functions from a contrib module into core
* Addition of peeking functions for changeset extraction.
* revised error messages
* revised comments
* ...

I've followed Robert's wishes with generalizing the replication slot
interface to not only work for changeset generation, but also streaming
rep - not sure whether that was the right choice, it's been more work
than I expected blocking things a bit but we're there now....
There's no clientside support included except as in the pg_receivexlog
hack attached as the last patch, but I also have tested it via streaming
rep and it mostly works (minus a hot_standby_feedback bug, will report
tomorrow).

What I think is missing:
* The user docs need more work, even though we're in a much better state
than before.
* Replication slots are stored in binary files. I think it might make
sense to store them as text files instead, for easier
extensibility. Especially since we want to use them for streaming rep,
I am pretty sure new attributes will soon come. I don't think it's
critical enough performancwise to store them in binary.
* Contrary to what Robert and I'd discussed I've named the SQL functions
outputting changes decoding_slot_(get|peek)_[binary_]changes instead of
decoding_stream_* - I can change that, but the SQL functions don't
actually support streaming, so I thought that might be
confusing. Opinions?
* Robert complained earlier about the way historical catalog snapshots
are forced in tqual.c - I don't really know yet what the better way
would be here.
* Some functionality probably needs to move between the patches - it's a
bit hard to see where the best boundaries are.

The sources are in my git tree at:
http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
branch xlog-decoding-rebasing-remapping.

The last two patches are *not* indendet to be actually applied, but are
useful for testing.

If you want to test, you'll need a clean initdb, set wal_level=logical
and max_replication_slots>0. There's a example SQL session showing how
things can be used....

Testing, Review, Questions welcome!

Andres Freund

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

This 0001 patch, to log running transactions more frequently, has been
pending for a long time now, and I haven't heard any objections, so
I've gone ahead and committed that part.

...Robert

Review of patch 0002:

- I think you should just regard ReplicationSlotCtlLock as protecting
the "name" and "active" flags of every slot. ReplicationSlotCreate()
would then not need to mess with the spinlocks at all, and
ReplicationSlotAcquire and ReplicationSlotDrop get a bit simpler too I
think. Functions like ReplicationSlotsComputeRequiredXmin() can
acquire this lock in shared mode and only lock the slots that are
actually active, instead of all of them.

- If you address /* FIXME: apply sanity checking to slot name */, then
I think that also addresses /* XXX: do we want to use truncate
identifier instead? */. In other words, let's just error out if the
name is too long. I'm not sure what other sanity checking is needed
here; maybe just restrict it to 7-bit characters to avoid problems
with encodings for individual databases varying.

- ReplicationSlotAcquire probably needs to ignore slots that are not active.

- ReplicationSlotAcquire should be tweaked so that the code that holds
the spinlock is more self-contained. If you adopt the above-proposed
recasting of ReplicationSlotCtlLock, then the part that holds the
spinlock can probably look like this: SpinLockAcquire(&slot->mutex);
was_active = slot->active; slot->active = true;
SpinLockRelease(&slot->mutex), which looks quite a bit safer.

- If there's a coding rule that slot->database can't be changed while
the slot is active, then the check to make sure that the user isn't
trying to bind to a slot with a mis-matching database could be done
before the code described in the previous point, avoiding the need to
go back and release the resource.

- I think the critical section in ReplicationSlotDrop is bogus. If
DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't
gone.

- cancel_before_shmem_exit is only guaranteed to remove the
most-recently-added callback.

- Why does ReplicationSlotsComputeRequiredXmin() need to hold
ProcArrayLock at all?

- ReplicationSlotsComputeRequiredXmin scarcely needs to hold the
spinlock while it does all of those gyrations. It can just acquire
the spinlock, copy the three fields needed into local variables, and
release the spinlock. The rest can be worked out afterwards.
Similarly in ReplicationSlotsComputeRequiredXmin.

- A comment in KillSlot wonders whether locking is required. I say
yes. It's safe to take lwlocks and spinlocks during shmem exit, and
failing to do so seems like a recipe for subtle corner-case bugs.

- pg_get_replication_slots() wonders what permissions we require. I
don't know that any special permissions are needed here; the data
we're exposing doesn't appear to be sensitive. Unless I'm missing
something?

- PG_STAT_GET_LOGICAL_DECODING_SLOTS_COLS has a leftover "logical" in its name.

- There seems to be no interface to acquire or release slots from
either SQL or the replication protocol, nor any way for a client of
this code to update its slot details. The value of
catalog_xmin/data_xmin vs. effective_catalog_xmin/effective_data_xmin
is left to the imagination.

...Robert

Hi,

On 2014-01-15 13:28:25 -0500, Robert Haas wrote:
> - I think you should just regard ReplicationSlotCtlLock as protecting
> the "name" and "active" flags of every slot. ReplicationSlotCreate()
> would then not need to mess with the spinlocks at all, and
> ReplicationSlotAcquire and ReplicationSlotDrop get a bit simpler too I
> think. Functions like ReplicationSlotsComputeRequiredXmin() can
> acquire this lock in shared mode and only lock the slots that are
> actually active, instead of all of them.

I first thought you meant that we should get rid of the spinlock, but
after rereading I think you just mean that ->name, ->active, ->in_use
are only allowed to change while holding the lwlock exclusively so we
don't need to spinlock in those cases? If so, yes, that works for me.

> - If you address /* FIXME: apply sanity checking to slot name */, then
> I think that also addresses /* XXX: do we want to use truncate
> identifier instead? */. In other words, let's just error out if the
> name is too long. I'm not sure what other sanity checking is needed
> here; maybe just restrict it to 7-bit characters to avoid problems
> with encodings for individual databases varying.

Yea, erroring out seems like a good idea. But I think we need to
restrict slot names a bit more than that, given they are used as
filenames... We could instead name the files using the slot's offset,
but I'd prefer to not go that route.

> - ReplicationSlotAcquire probably needs to ignore slots that are not active.

Not sure what you mean? If the slot isn't in_use we'll skip it in the loop.

> - If there's a coding rule that slot->database can't be changed while
> the slot is active, then the check to make sure that the user isn't
> trying to bind to a slot with a mis-matching database could be done
> before the code described in the previous point, avoiding the need to
> go back and release the resource.

I don't think slot->database should be allowed to change at all...

> - I think the critical section in ReplicationSlotDrop is bogus. If
> DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't
> gone.

Well, if delete slot fails, we don't really know at which point it
failed which means that the on-disk state might not correspond to the
in-memory state. I don't see a point in adding code trying to handle
that case correctly...

> - cancel_before_shmem_exit is only guaranteed to remove the
> most-recently-added callback.

Yea :(. I think that's safe for the current usages but seems mighty
fragile... Not sure what to do about it. Just register KillSlot once and
keep it registered?

> - Why does ReplicationSlotsComputeRequiredXmin() need to hold
> ProcArrayLock at all?

There's reasoning, but I just noticed that it's basis might be flawed
anyway :(.
When starting changeset extraction in a new slot we need to guarantee
that we only start decoding records we know the catalog tuples haven't
been removed for.
So, when creating the slot I've so far done a GetOldestXmin() and used
that to check against xl_running_xact->oldestRunningXid. But
GetOldestXmin() can go backwards...

I'll think a bit and try to simplify this.

> - ReplicationSlotsComputeRequiredXmin scarcely needs to hold the
> spinlock while it does all of those gyrations. It can just acquire
> the spinlock, copy the three fields needed into local variables, and
> release the spinlock. The rest can be worked out afterwards.
> Similarly in ReplicationSlotsComputeRequiredXmin.

Yea, will change.

> - A comment in KillSlot wonders whether locking is required. I say
> yes. It's safe to take lwlocks and spinlocks during shmem exit, and
> failing to do so seems like a recipe for subtle corner-case bugs.

I agree that it's safe to use spinlocks, but lwlocks? What if we are
erroring out while holding an lwlock? Code that runs inside a
TransactionCommand is protected against that, but if not ProcKill()
which invokes LWLockReleaseAll() runs pretty late in the teardown
process...

> - pg_get_replication_slots() wonders what permissions we require. I
> don't know that any special permissions are needed here; the data
> we're exposing doesn't appear to be sensitive. Unless I'm missing
> something?

I don't see a problem either, but it seems others have -
pg_stat_replication only displays minor amounts of information if one
doesn't have the replication privilege... Not sure what the reasoning
there is, and whether it applies here as well.

> - There seems to be no interface to acquire or release slots from
> either SQL or the replication protocol, nor any way for a client of
> this code to update its slot details.

I don't think either ever needs to do that - START_TRANSACTION SLOT slot
...; and decoding_slot_*changes will acquire/release for them while
active. What would the usecase be to allow them to be acquired from SQL?

The slot details get updates by the respective replication code. For
streaming rep, that should happen via reply and feedback messages. For
changeset extraction it happens when LogicalConfirmReceivedLocation() is
called; the walsender interface does that using reply messages, the SQL
interface calls it when finished (unless you use the _peek_ functions).

> The value of
> catalog_xmin/data_xmin vs. effective_catalog_xmin/effective_data_xmin
> is left to the imagination.

There's a comment about them in a following patch. Basically the reason
is that for changeset extraction we cannot adjust the in-memory value
before we know the changed slot status is safely synced to
disk. Otherwise a client could restart streaming at a LSN where the
corresponding catalog details are gone since it's not prevented by the
slot anymore.

That could be done by just holding some lock forbidding the global xmin
value to be recomputing while writing to disk, but that seems awfully
heavy-handed. So the protocol is:
1) update ->catalog_xmin to the new xmin,
2) sync slot to disk
3) set ->effective_catalog_xmin = ->catalog_xmin
4) ReplicationSlotsComputeRequiredXmin()

Since ComputeRequiredXmin() only looks at effective_catalog_xmin that
guarantees that the global xmin horizon doesn't increase before the the
slot has been synced to disk. If we crash after 2,
StartupReplicationSlots() simply sets effective_catalog_xmin =
catalog_xmin, we know it's safely on disk now since we've just read it
from there.

Thanks for committing 0001!

Regards,

Andres

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

On Wed, Jan 15, 2014 at 3:39 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-01-15 13:28:25 -0500, Robert Haas wrote:
>> - I think you should just regard ReplicationSlotCtlLock as protecting
>> the "name" and "active" flags of every slot. ReplicationSlotCreate()
>> would then not need to mess with the spinlocks at all, and
>> ReplicationSlotAcquire and ReplicationSlotDrop get a bit simpler too I
>> think. Functions like ReplicationSlotsComputeRequiredXmin() can
>> acquire this lock in shared mode and only lock the slots that are
>> actually active, instead of all of them.
>
> I first thought you meant that we should get rid of the spinlock, but
> after rereading I think you just mean that ->name, ->active, ->in_use
> are only allowed to change while holding the lwlock exclusively so we
> don't need to spinlock in those cases? If so, yes, that works for me.

Yeah, that's about what I had in mind.

>> - If you address /* FIXME: apply sanity checking to slot name */, then
>> I think that also addresses /* XXX: do we want to use truncate
>> identifier instead? */. In other words, let's just error out if the
>> name is too long. I'm not sure what other sanity checking is needed
>> here; maybe just restrict it to 7-bit characters to avoid problems
>> with encodings for individual databases varying.
>
> Yea, erroring out seems like a good idea. But I think we need to
> restrict slot names a bit more than that, given they are used as
> filenames... We could instead name the files using the slot's offset,
> but I'd prefer to not go that route.

OK. Well, add some code, then. :-)

>> - ReplicationSlotAcquire probably needs to ignore slots that are not active.
> Not sure what you mean? If the slot isn't in_use we'll skip it in the loop.

active != in_use.

I suppose your point is that the slot can't be in_use if it's not also
active. Maybe it would be better to get rid of active/in_use and have
three states: REPLSLOT_CONNECTED, REPLSLOT_NOT_CONNECTED,
REPLSLOT_FREE. Or something like that.

>> - If there's a coding rule that slot->database can't be changed while
>> the slot is active, then the check to make sure that the user isn't
>> trying to bind to a slot with a mis-matching database could be done
>> before the code described in the previous point, avoiding the need to
>> go back and release the resource.
>
> I don't think slot->database should be allowed to change at all...

Well, it can if the slot is dropped and a new one created.

>> - I think the critical section in ReplicationSlotDrop is bogus. If
>> DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't
>> gone.
>
> Well, if delete slot fails, we don't really know at which point it
> failed which means that the on-disk state might not correspond to the
> in-memory state. I don't see a point in adding code trying to handle
> that case correctly...

Deleting the slot should be an atomic operation. There's some
critical point before which the slot will be picked up by recovery and
after which it won't. You either did that operation, or not, and can
adjust the in-memory state accordingly.

>> - cancel_before_shmem_exit is only guaranteed to remove the
>> most-recently-added callback.
>
> Yea :(. I think that's safe for the current usages but seems mighty
> fragile... Not sure what to do about it. Just register KillSlot once and
> keep it registered?

Yep. Use a module-private flag to decide whether it needs to do anything.

>> - A comment in KillSlot wonders whether locking is required. I say
>> yes. It's safe to take lwlocks and spinlocks during shmem exit, and
>> failing to do so seems like a recipe for subtle corner-case bugs.
>
> I agree that it's safe to use spinlocks, but lwlocks? What if we are
> erroring out while holding an lwlock? Code that runs inside a
> TransactionCommand is protected against that, but if not ProcKill()
> which invokes LWLockReleaseAll() runs pretty late in the teardown
> process...

Hmm. I guess it'd be fine to decide that a connected slot can be
marked not-connected without the lock. I think you'd want a rule that
a slot can't be freed except when it's not-connected; otherwise, you
might end up marking the slot not-connected after someone else had
already recycled it for an unrelated purpose (drop slot, create new
slot).

>> - There seems to be no interface to acquire or release slots from
>> either SQL or the replication protocol, nor any way for a client of
>> this code to update its slot details.
>
> I don't think either ever needs to do that - START_TRANSACTION SLOT slot
> ...; and decoding_slot_*changes will acquire/release for them while
> active. What would the usecase be to allow them to be acquired from SQL?

My point isn't so much about SQL as that with just this patch I don't
see any way for anyone to ever acquire a slot for anything, ever. So
I think there's a piece missing, or three.

> The slot details get updates by the respective replication code. For
> streaming rep, that should happen via reply and feedback messages. For
> changeset extraction it happens when LogicalConfirmReceivedLocation() is
> called; the walsender interface does that using reply messages, the SQL
> interface calls it when finished (unless you use the _peek_ functions).

Right, but where is this code? I don't see this updating the reply
and feedback message processing code to touch slots. Did I miss that?

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

Hi,

On 2014-01-16 09:34:51 -0500, Robert Haas wrote:
> >> - ReplicationSlotAcquire probably needs to ignore slots that are not active.
> > Not sure what you mean? If the slot isn't in_use we'll skip it in the loop.
>
> active != in_use.
>
> I suppose your point is that the slot can't be in_use if it's not also
> active.

Yes. There's asserts to that end...

> Maybe it would be better to get rid of active/in_use and have
> three states: REPLSLOT_CONNECTED, REPLSLOT_NOT_CONNECTED,
> REPLSLOT_FREE. Or something like that.

Hm. Color me unenthusiastic. If you feel strongly I can change it, but
otherwise not.

> >> - If there's a coding rule that slot->database can't be changed while
> >> the slot is active, then the check to make sure that the user isn't
> >> trying to bind to a slot with a mis-matching database could be done
> >> before the code described in the previous point, avoiding the need to
> >> go back and release the resource.
> >
> > I don't think slot->database should be allowed to change at all...
>
> Well, it can if the slot is dropped and a new one created.

Well. That obviously requires the lwlock to be acquired...

> >> - I think the critical section in ReplicationSlotDrop is bogus. If
> >> DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't
> >> gone.
> >
> > Well, if delete slot fails, we don't really know at which point it
> > failed which means that the on-disk state might not correspond to the
> > in-memory state. I don't see a point in adding code trying to handle
> > that case correctly...
>
> Deleting the slot should be an atomic operation. There's some
> critical point before which the slot will be picked up by recovery and
> after which it won't. You either did that operation, or not, and can
> adjust the in-memory state accordingly.

I am not sure I understand that point. We can either update the
in-memory bit before performing the on-disk operations or
afterwards. Either way, there's a way to be inconsistent if the disk
operation fails somewhere inbetween (it might fail but still have
deleted the file/directory!). The normal way to handle that in other
places is PANICing when we don't know so we recover from the on-disk
state.
I really don't see the problem here? Code doesn't get more robust by
doing s/PANIC/ERROR/, rather the contrary. It takes extra smarts to only
ERROR, often that's not warranted.

> >> - A comment in KillSlot wonders whether locking is required. I say
> >> yes. It's safe to take lwlocks and spinlocks during shmem exit, and
> >> failing to do so seems like a recipe for subtle corner-case bugs.
> >
> > I agree that it's safe to use spinlocks, but lwlocks? What if we are
> > erroring out while holding an lwlock? Code that runs inside a
> > TransactionCommand is protected against that, but if not ProcKill()
> > which invokes LWLockReleaseAll() runs pretty late in the teardown
> > process...
>
> Hmm. I guess it'd be fine to decide that a connected slot can be
> marked not-connected without the lock.

I now acquire the spinlock since that has to work, or we have much worse
problems... That guarantees that other backends see the value as well.

> I think you'd want a rule that
> a slot can't be freed except when it's not-connected; otherwise, you
> might end up marking the slot not-connected after someone else had
> already recycled it for an unrelated purpose (drop slot, create new
> slot).

Yea, that rule is there. Otherwise we'd get in great trouble.

> >> - There seems to be no interface to acquire or release slots from
> >> either SQL or the replication protocol, nor any way for a client of
> >> this code to update its slot details.
> >
> > I don't think either ever needs to do that - START_TRANSACTION SLOT slot
> > ...; and decoding_slot_*changes will acquire/release for them while
> > active. What would the usecase be to allow them to be acquired from SQL?
>
> My point isn't so much about SQL as that with just this patch I don't
> see any way for anyone to ever acquire a slot for anything, ever. So
> I think there's a piece missing, or three.

The slot is acquired by code using the slot. So when START_TRANSACTION
SLOT ... (in contrast to a START_TRANSACTION without SLOT) is sent,
walsender.c does an ReplicationSlotAcquire(cmd->slotname) in
StartReplication() and releases it after it has finished.

> > The slot details get updates by the respective replication code. For
> > streaming rep, that should happen via reply and feedback
> > messages. For changeset extraction it happens when
> > LogicalConfirmReceivedLocation() is called; the walsender interface
> > does that using reply messages, the SQL interface calls it when
> > finished (unless you use the _peek_ functions).
>
> Right, but where is this code? I don't see this updating the reply
> and feedback message processing code to touch slots. Did I miss that?

It's in "wal_decoding: logical changeset extraction walsender interface"
currently :(. Splitting the streaming replication part of that patch off
isn't easy...

Greetings,

Andres Freund

On 01/16/2014 02:28 AM, Robert Haas wrote:
> - If you address /* FIXME: apply sanity checking to slot name */, then
> I think that also addresses /* XXX: do we want to use truncate
> identifier instead? */. In other words, let's just error out if the
> name is too long. I'm not sure what other sanity checking is needed
> here; maybe just restrict it to 7-bit characters to avoid problems
> with encodings for individual databases varying.

It's a common misunderstanding that restricting to 7-bit solves encoding
issues.

Thanks to the joy that is SHIFT_JIS, we must also disallow the backslash
and tilde characters.

Anybody who actually uses SHIFT_JIS as an operational encoding, rather
than as an input/output encoding, is into pain and suffering. Personally
I'd be quite happy to see it supported as client_encoding, but forbidden
as a server-side encoding. That's not the case right now - so since we
support it, we'd better guard against its quirks.

slotnames can't be regular identifiers, because they might contain chars
not valid in another DB's encoding. So lets just restrict them to
[a-zA-Z0-9_ -] and be done with it.

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

On Thu, Jan 16, 2014 at 10:15 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
> Anybody who actually uses SHIFT_JIS as an operational encoding, rather
> than as an input/output encoding, is into pain and suffering. Personally
> I'd be quite happy to see it supported as client_encoding, but forbidden
> as a server-side encoding. That's not the case right now - so since we
> support it, we'd better guard against its quirks.

I think that *is* the case right now. pg_wchar.h sayeth:

/* followings are for client encoding only */
PG_SJIS, /* Shift JIS
(Winindows-932) */
PG_BIG5, /* Big5 (Windows-950) */
PG_GBK, /* GBK (Windows-936) */

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

On Thu, Jan 16, 2014 at 9:54 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Maybe it would be better to get rid of active/in_use and have
>> three states: REPLSLOT_CONNECTED, REPLSLOT_NOT_CONNECTED,
>> REPLSLOT_FREE. Or something like that.
>
> Hm. Color me unenthusiastic. If you feel strongly I can change it, but
> otherwise not.

I found the active/in_use distinction confusing; I thought one
three-state flag rather than two Booleans might be clearer. But I
might be able to just suck it up.

>> >> - If there's a coding rule that slot->database can't be changed while
>> >> the slot is active, then the check to make sure that the user isn't
>> >> trying to bind to a slot with a mis-matching database could be done
>> >> before the code described in the previous point, avoiding the need to
>> >> go back and release the resource.
>> >
>> > I don't think slot->database should be allowed to change at all...
>>
>> Well, it can if the slot is dropped and a new one created.
>
> Well. That obviously requires the lwlock to be acquired...

Right, so the point of this comment originally was you had some logic
that could be moved sooner to avoid having to undo so much on a
failure.

>> >> - I think the critical section in ReplicationSlotDrop is bogus. If
>> >> DeleteSlot() fails, we scarcely need to PANIC. The slot just isn't
>> >> gone.
>> >
>> > Well, if delete slot fails, we don't really know at which point it
>> > failed which means that the on-disk state might not correspond to the
>> > in-memory state. I don't see a point in adding code trying to handle
>> > that case correctly...
>>
>> Deleting the slot should be an atomic operation. There's some
>> critical point before which the slot will be picked up by recovery and
>> after which it won't. You either did that operation, or not, and can
>> adjust the in-memory state accordingly.
>
> I am not sure I understand that point. We can either update the
> in-memory bit before performing the on-disk operations or
> afterwards. Either way, there's a way to be inconsistent if the disk
> operation fails somewhere inbetween (it might fail but still have
> deleted the file/directory!). The normal way to handle that in other
> places is PANICing when we don't know so we recover from the on-disk
> state.
> I really don't see the problem here? Code doesn't get more robust by
> doing s/PANIC/ERROR/, rather the contrary. It takes extra smarts to only
> ERROR, often that's not warranted.

People get cranky when the database PANICs because of a filesystem
failure. We should avoid that, especially when it's trivial to do so.
The update to shared memory should be done second and should be set
up to be no-fail.

>> > The slot details get updates by the respective replication code. For
>> > streaming rep, that should happen via reply and feedback
>> > messages. For changeset extraction it happens when
>> > LogicalConfirmReceivedLocation() is called; the walsender interface
>> > does that using reply messages, the SQL interface calls it when
>> > finished (unless you use the _peek_ functions).
>>
>> Right, but where is this code? I don't see this updating the reply
>> and feedback message processing code to touch slots. Did I miss that?
>
> It's in "wal_decoding: logical changeset extraction walsender interface"
> currently :(. Splitting the streaming replication part of that patch off
> isn't easy...

Ack. I was hoping to work through these patches one at a time, but
that's not going to work if they are interdependent to that degree.

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

On 01/18/2014 09:31 PM, Robert Haas wrote:
> On Thu, Jan 16, 2014 at 10:15 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Anybody who actually uses SHIFT_JIS as an operational encoding, rather
>> than as an input/output encoding, is into pain and suffering. Personally
>> I'd be quite happy to see it supported as client_encoding, but forbidden
>> as a server-side encoding. That's not the case right now - so since we
>> support it, we'd better guard against its quirks.
>
> I think that *is* the case right now. pg_wchar.h sayeth:
>
> /* followings are for client encoding only */
> PG_SJIS, /* Shift JIS
> (Winindows-932) */
> PG_BIG5, /* Big5 (Windows-950) */
> PG_GBK, /* GBK (Windows-936) */

Perfect - that makes ASCII-only just fine, IMO.

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> On Thu, Jan 16, 2014 at 10:15 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Anybody who actually uses SHIFT_JIS as an operational encoding, rather
>> than as an input/output encoding, is into pain and suffering. Personally
>> I'd be quite happy to see it supported as client_encoding, but forbidden
>> as a server-side encoding. That's not the case right now - so since we
>> support it, we'd better guard against its quirks.

> I think that *is* the case right now.

SHIFT_JIS is not and never will be allowed as a server encoding,
precisely because it has multi-byte characters of which some bytes could
be taken for ASCII. The same is true of our other client-only encodings.

regards, tom lane

On 01/18/2014 02:31 PM, Robert Haas wrote:
> On Thu, Jan 16, 2014 at 10:15 PM, Craig Ringer <craig(at)2ndquadrant(dot)com> wrote:
>> Anybody who actually uses SHIFT_JIS as an operational encoding, rather
>> than as an input/output encoding, is into pain and suffering. Personally
>> I'd be quite happy to see it supported as client_encoding, but forbidden
>> as a server-side encoding. That's not the case right now - so since we
>> support it, we'd better guard against its quirks.
>
> I think that *is* the case right now. pg_wchar.h sayeth:
>
> /* followings are for client encoding only */
> PG_SJIS, /* Shift JIS
> (Winindows-932) */

while you have that file open: s/Winindows-932/Windows-932 maybe?

Stefan

On 2014-01-18 08:35:47 -0500, Robert Haas wrote:
> > I am not sure I understand that point. We can either update the
> > in-memory bit before performing the on-disk operations or
> > afterwards. Either way, there's a way to be inconsistent if the disk
> > operation fails somewhere inbetween (it might fail but still have
> > deleted the file/directory!). The normal way to handle that in other
> > places is PANICing when we don't know so we recover from the on-disk
> > state.
> > I really don't see the problem here? Code doesn't get more robust by
> > doing s/PANIC/ERROR/, rather the contrary. It takes extra smarts to only
> > ERROR, often that's not warranted.
>
> People get cranky when the database PANICs because of a filesystem
> failure. We should avoid that, especially when it's trivial to do so.
> The update to shared memory should be done second and should be set
> up to be no-fail.

I don't see how that would help. If we fail during unlink/rmdir, we
don't really know at which point we failed. Just keeping the slot in
memory, won't help us in any way - we'll continue to reserve resources
while the slot is half-gone.
I don't think trying to handle errors we don't understand and we don't
routinely expect actually improves robustness. It just leads to harder
to diagnose errors. It's not like the cases here are likely to be caused
by anthything but severe admin failure like removing the write
permissions of the postgres directory while the server is running. Or do
you see more valid causes?

Greetings,

Andres Freund

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

On Wed, Jan 22, 2014 at 9:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-01-18 08:35:47 -0500, Robert Haas wrote:
>> > I am not sure I understand that point. We can either update the
>> > in-memory bit before performing the on-disk operations or
>> > afterwards. Either way, there's a way to be inconsistent if the disk
>> > operation fails somewhere inbetween (it might fail but still have
>> > deleted the file/directory!). The normal way to handle that in other
>> > places is PANICing when we don't know so we recover from the on-disk
>> > state.
>> > I really don't see the problem here? Code doesn't get more robust by
>> > doing s/PANIC/ERROR/, rather the contrary. It takes extra smarts to only
>> > ERROR, often that's not warranted.
>>
>> People get cranky when the database PANICs because of a filesystem
>> failure. We should avoid that, especially when it's trivial to do so.
>> The update to shared memory should be done second and should be set
>> up to be no-fail.
>
> I don't see how that would help. If we fail during unlink/rmdir, we
> don't really know at which point we failed.

This doesn't make sense to me. unlink/rmdir are atomic operations.

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

Hi,

Attached is v7.1 of the patchset with (among others) the following
changes:
* rebase to master
* split the slot support for streaming replication into a separate
patch, early in the series
* slot names are now limited to /[a-z0-9_]{1,NAMEDATALEN-1}/
* computation of the initial xmin for changeset extraction is now done
with an extra routine getting rid of races around GetOldestXmin()
going forwards and then backwards and getting rid of an additional
parameter to GetOldestXmin().
* slot locking is rejiggered according to Robert's suggestions
* comment improvements
* sgml documentation improvements
* ...

I think sgml the documentation is in a reasonable shape now, I'd
appreciate somebody else having a look. I think a bit more effort is
required in protocol.sgml.

Greetings,

Andres Freund

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

On 2014-01-22 10:14:27 -0500, Robert Haas wrote:
> On Wed, Jan 22, 2014 at 9:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > On 2014-01-18 08:35:47 -0500, Robert Haas wrote:
> >> > I am not sure I understand that point. We can either update the
> >> > in-memory bit before performing the on-disk operations or
> >> > afterwards. Either way, there's a way to be inconsistent if the disk
> >> > operation fails somewhere inbetween (it might fail but still have
> >> > deleted the file/directory!). The normal way to handle that in other
> >> > places is PANICing when we don't know so we recover from the on-disk
> >> > state.
> >> > I really don't see the problem here? Code doesn't get more robust by
> >> > doing s/PANIC/ERROR/, rather the contrary. It takes extra smarts to only
> >> > ERROR, often that's not warranted.
> >>
> >> People get cranky when the database PANICs because of a filesystem
> >> failure. We should avoid that, especially when it's trivial to do so.
> >> The update to shared memory should be done second and should be set
> >> up to be no-fail.
> >
> > I don't see how that would help. If we fail during unlink/rmdir, we
> > don't really know at which point we failed.
>
> This doesn't make sense to me. unlink/rmdir are atomic operations.

Yes, individual operations should be, but you cannot be sure whether a
rename()/unlink() will survive a crash until the directory is
fsync()ed. So, what is one going to do if the unlink suceeded, but the
fsync didn't?

Deletion currently works like:
if (rename(path, tmppath) != 0)
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not rename \"%s\" to \"%s\": %m",
path, tmppath)));

/* make sure no partial state is visible after a crash */
fsync_fname(tmppath, false);
fsync_fname("pg_replslot", true);

if (!rmtree(tmppath, true))
{
ereport(ERROR,
(errcode_for_file_access(),
errmsg("could not remove directory \"%s\": %m",
tmppath)));
}

If we fail between the rename() and the fsync_fname() we don't really
know which state we are in. We'd also have to add code to handle
incomplete slot directories, which currently only exists for startup, to
other places.

Greetings,

Andres Freund

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

On Wed, Jan 22, 2014 at 10:48 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Yes, individual operations should be, but you cannot be sure whether a
> rename()/unlink() will survive a crash until the directory is
> fsync()ed. So, what is one going to do if the unlink suceeded, but the
> fsync didn't?

Well, apparently, one is going to PANIC and reinitialize the system.
I presume that upon reinitialization we'll decide that the slot is
gone, and thus won't recreate it in shared memory. Of course, if the
entire system suffers a hard power failure after that and before the
directory is succesfully fsync'd, then the slot could reappear on the
next startup. Which is also exactly what would happen if we removed
the slot from shared memory after doing the unlink, and then the
system suffered a hard power failure before the directory contents
made it to disk. Except that we also panicked.

In the case of shared buffers, the way we handle fsync failures is by
not allowing the system to checkpoint until all of the fsyncs succeed.
If there's an OS-level reset before that happens, WAL replay will
perform the same buffer modifications over again and the next
checkpoint will again try to flush them to disk and will not complete
unless it does. That forms a closed system where we never advance the
redo pointer over the covering WAL record until the changes it covers
are on the disk. But I don't think this code has any similar
interlock; if it does, I missed it.

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

Hi,

On 2014-01-22 13:00:44 -0500, Robert Haas wrote:
> Well, apparently, one is going to PANIC and reinitialize the system.
> I presume that upon reinitialization we'll decide that the slot is
> gone, and thus won't recreate it in shared memory.

Yea, and if it's half-gone we'll continue deletion. And since yesterday
evening we'll even fsync things during startup to handle scenarios
similar to 20140122162115(dot)GL21170(at)alap3(dot)anarazel(dot)de .

> Of course, if the entire system suffers a hard power failure after that and before the
> directory is succesfully fsync'd, then the slot could reappear on the
> next startup. Which is also exactly what would happen if we removed
> the slot from shared memory after doing the unlink, and then the
> system suffered a hard power failure before the directory contents
> made it to disk. Except that we also panicked.

Yes, but that could only happen as long as no relevant data has been
lost since we hold relevant locks during this.

> In the case of shared buffers, the way we handle fsync failures is by
> not allowing the system to checkpoint until all of the fsyncs succeed.

I don't think shared buffers fsyncs are the apt comparison. It's more
something like UpdateControlFile(). Which PANICs.

I really don't get why you fight PANICs in general that much. There are
some nasty PANICs in postgres which can happen in legitimate situations,
which should be made to fail more gracefully, but this surely isn't one
of them. We're doing rename(), unlink() and rmdir(). That's it.
We should concentrate on the ones that legitimately can happen, not the
ones created by an admin running a chmod -R 000 . ; rm -rf $PGDATA or
mount -o remount,ro /. We don't increase reliability by a bit adding
codepaths that will never get tested.

> If there's an OS-level reset before that happens, WAL replay will
> perform the same buffer modifications over again and the next
> checkpoint will again try to flush them to disk and will not complete
> unless it does. That forms a closed system where we never advance the
> redo pointer over the covering WAL record until the changes it covers
> are on the disk. But I don't think this code has any similar
> interlock; if it does, I missed it.

No, it doesn't (until the first rename() at least), but the number of
failure scenarios is far smaller.

Greetings,

Andres Freund

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

On Thu, Jan 23, 2014 at 7:05 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> I don't think shared buffers fsyncs are the apt comparison. It's more
> something like UpdateControlFile(). Which PANICs.
>
> I really don't get why you fight PANICs in general that much. There are
> some nasty PANICs in postgres which can happen in legitimate situations,
> which should be made to fail more gracefully, but this surely isn't one
> of them. We're doing rename(), unlink() and rmdir(). That's it.
> We should concentrate on the ones that legitimately can happen, not the
> ones created by an admin running a chmod -R 000 . ; rm -rf $PGDATA or
> mount -o remount,ro /. We don't increase reliability by a bit adding
> codepaths that will never get tested.

Sorry, I don't buy it. Lots of people I know have stories that go
like this "$HORRIBLE happened, and PostgreSQL kept on running, and it
didn't even lose my data!", where $HORRIBLE may be variously that the
disk filled up, that disk writes started failing with I/O errors, that
somebody changed the permissions on the data directory inadvertently,
that the entire data directory got removed, and so on. I've been
through some of those scenarios myself, and the care and effort that's
been put into failure modes has saved my bacon more than a few times,
too. We *do* increase reliability by worrying about what will happen
even in code paths that very rarely get exercised. It's certainly
true that our bug count there is higher there than for the parts of
our code that get exercised more regularly, but it's also lower than
it would be if we didn't make the effort, and the dividend that we get
from that effort is that we have a well-deserved reputation for
reliability.

I think it's completely unacceptable for the failure of routine
filesystem operations to result in a PANIC. I grant you that we have
some existing cases where that can happen (like UpdateControlFile),
but that doesn't mean we should add more. Right this very minute
there is massive bellyaching on a nearby thread caused by the fact
that a full disk condition while writing WAL can PANIC the server,
while on this thread at the very same time you're arguing that adding
more ways for a full disk to cause PANICs won't inconvenience anyone.
The other thread is right, and your argument here is wrong. We have
been able to - and have taken the time to - fix comparable problems in
other cases, and we should do the same thing here.

As for why I fight PANICs so much in general, there are two reasons.
First, I believe that to be project policy. I welcome correction if I
have misinterpreted our stance in that area. Second, I have
encountered a few situations where customers had production servers
that repeatedly PANICked due to some bug or other. If I've ever
encountered angrier customers, I can't remember when. A PANIC is no
big deal when it happens on your development box, but when it happens
on a machine with 100 users connected to it, it's a big deal,
especially if a single underlying cause makes it happen over and over
again.

I think we should be devoting our time to figuring how to improve
this, not whether to improve it.

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

On 2014-01-23 11:50:57 -0500, Robert Haas wrote:
> On Thu, Jan 23, 2014 at 7:05 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > I don't think shared buffers fsyncs are the apt comparison. It's more
> > something like UpdateControlFile(). Which PANICs.
> >
> > I really don't get why you fight PANICs in general that much. There are
> > some nasty PANICs in postgres which can happen in legitimate situations,
> > which should be made to fail more gracefully, but this surely isn't one
> > of them. We're doing rename(), unlink() and rmdir(). That's it.
> > We should concentrate on the ones that legitimately can happen, not the
> > ones created by an admin running a chmod -R 000 . ; rm -rf $PGDATA or
> > mount -o remount,ro /. We don't increase reliability by a bit adding
> > codepaths that will never get tested.
>
> Sorry, I don't buy it. Lots of people I know have stories that go
> like this "$HORRIBLE happened, and PostgreSQL kept on running, and it
> didn't even lose my data!", where $HORRIBLE may be variously that the
> disk filled up, that disk writes started failing with I/O errors, that
> somebody changed the permissions on the data directory inadvertently,
> that the entire data directory got removed, and so on.

Especially the "not loosing data" imo is because postgres is
conservative with continuing in situations it doesn't know anything
about. Most prominently the cluster wide restart after a segfault.

> I've been
> through some of those scenarios myself, and the care and effort that's
> been put into failure modes has saved my bacon more than a few times,
> too. We *do* increase reliability by worrying about what will happen
> even in code paths that very rarely get exercised.

A part of thinking about them *is* restricting what happens in those
cases by keeping the possible states to worry about to a minimum.

Just splapping on an ERROR instead of PANIC can make things much
worse. Not releasing space until a restart, without a chance to do
anything about it because we failed to properly release the in-memory
slot will just make the problem bigger because now the cleanup might
take a week (VACUUM FULLing the entire cluster?).

> I think it's completely unacceptable for the failure of routine
> filesystem operations to result in a PANIC. I grant you that we have
> some existing cases where that can happen (like UpdateControlFile),
> but that doesn't mean we should add more. Right this very minute
> there is massive bellyaching on a nearby thread caused by the fact
> that a full disk condition while writing WAL can PANIC the server,
> while on this thread at the very same time you're arguing that adding
> more ways for a full disk to cause PANICs won't inconvenience anyone.

A full disk won't cause any of the problems for the case we're
discussing, will it? We're just doing rename(), unlink(), rmdir() here,
all should succeed while the FS is full (afair rename() does on all
common FSs because inodes are kept separately).

> The other thread is right, and your argument here is wrong. We have
> been able to - and have taken the time to - fix comparable problems in
> other cases, and we should do the same thing here.

I don't think the WAL case is comparable at all. ENOSPC is something
expected that can happen during normal operation and doesn't include
malintended operator and is reasonably easy to test. unlink() or fsync()
randomly failing is not.
In fact, isn't the consequence out of that thread that we need a
significant amount of extra complexity to handle the case? We shouldn't
spend that effort for cases that don't deserve it because they are too
unlikely in practice.

And yes, there's not too many other places PANICing - because most can
rely on WAL handling those tricky cases for them...

> Second, I have
> encountered a few situations where customers had production servers
> that repeatedly PANICked due to some bug or other. If I've ever
> encountered angrier customers, I can't remember when. A PANIC is no
> big deal when it happens on your development box, but when it happens
> on a machine with 100 users connected to it, it's a big deal,
> especially if a single underlying cause makes it happen over and over
> again.

Sure. But blindly continuing and then, possibly quite a bit later,
loosing data, causing an outage that takes a long while to recover or
something isn't any better.

> I think we should be devoting our time to figuring how to improve
> this, not whether to improve it.

If you'd argue that creating a new slot should fail gracefull, ok, I can
relatively easily be convinced of that. But trying to handle failures in
the midst of deletion in cases that won't happen in reality is just
inviting trouble imo.

Greetings,

Andres Freund

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

Patch 0001:

+ errmsg("could not find free replication slot"),

Suggest: all replication slots are in use

+ elog(ERROR, "cannot aquire a slot while another slot
has been acquired");

Suggest: this backend has already acquired a replication slot

Or demote it to Assert(). I'm not really sure why this needs to be
checked in non-assert builds. I also wonder if we should use the
terminology "attach" instead of "acquire"; that pairs more naturally
with "release". Then the message, if we want more than an assert,
might be "this backend is already attached to a replication slot".

+ if (slot == NULL)
+ {
+ LWLockRelease(ReplicationSlotCtlLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_UNDEFINED_OBJECT),
+ errmsg("could not find replication
slot \"%s\"", name)));
+ }

The error will release the LWLock anyway; I'd get rid of the manual
LWLockRelease, and the braces. Similarly in ReplicationSlotDrop.

+ /* acquire spinlock so we can test and set ->active safely */
+ SpinLockAcquire(&slot->mutex);
+
+ if (slot->active)
+ {
+ SpinLockRelease(&slot->mutex);
+ LWLockRelease(ReplicationSlotCtlLock);
+ ereport(ERROR,
+ (errcode(ERRCODE_OBJECT_IN_USE),
+ errmsg("slot \"%s\" already active", name)));
+ }
+
+ /* we definitely have the slot, no errors possible anymore */
+ slot->active = true;
+ MyReplicationSlot = slot;
+ SpinLockRelease(&slot->mutex);

This doesn't need the LWLockRelease either. It does need the
SpinLockRelease, but as I think I noted previously, the right way to
write this is probably: SpinLockAcquire(&slot->mutex); was_active =
slot->active; slot->active = true; SpinLockRelease(&slot->mutex); if
(was_active) ereport(). MyReplicatonSlot = slot.

ReplicationSlotsComputeRequiredXmin still acquires ProcArrayLock, and
the comment "Provide interlock against concurrent recomputations"
doesn't seem adequate to me. I guess the idea here is that we regard
ProcArrayLock as protecting ReplicationSlotCtl->catalog_xmin and
ReplicationSlotCtl->data_xmin, but if that's the idea then we only
need to hold the lock during the time when we actually update those
values, not the loop where we compute them. Also, if that's the
design, maybe they should be part of PROC_HDR *ProcGlobal rather than
here. It seems weird to have some of the values protected by
ProcArrayLock live in a completely different data structure managed
almost entirely by some other part of the system.

It's pretty evident that what's currently patch #4 (only peg the xmin
horizon for catalog tables during logical decoding) needs to become
patch #1, because it doesn't make any sense to apply this before we do
that. I'm still not 100% confident in that approach, but maybe I'd
better try to look at it RSN and get confident, because too much of
the rest of what you've got here hangs on that to proceed without it.
Or to put all that another way, if for any reason we decide that the
separate catalog xmin stuff is not viable, the rest of this is going
to need a lot of rework, so we'd better sort that now rather than
later.

With respect to the synchronize-slots-to-disk stuff we're arguing
about on the other thread, I think the basic design problem here is
that you assume that you can change stuff in memory and then change
stuff on disk, without either set of changes being atomic. What I
think you need to do is making atomic actions on disk correspond to
atomic state changes in memory. IOW, suppose that creating a slot
consists of two steps: mkdir() + fsync(). Then I think what you need
to do is - do the mkdir(). If it errors out, fine. If it succeeds,
the mark the slot half-created. This is just an assignment so it can
done immediately after you learn that mkdir() worked with no risk of
an intervening failure. Then, try to fsync(). If it errors out, the
slot will get left in the half-created state. If it works, then
immediately mark the slot as fully created. Now, when the next guy
comes along and looks at the slot, he can tell what he needs to do.
Specifically, if the slot is half-created, and he wants to do anything
other than remove it, he's got to fsync() it first, and if that errors
out, so be it. The next access to the slot will merely find it still
half-created and simply try the fsync() yet again.

Alternatively, since nearly everything we're trying to do here is a
two-step operation - do something and then fsync - maybe we have a
more generic fsync-pending flag, and each slot operation checks that
and retries the fsync() if it's set. But it might be situation
dependent which thing we need to fsync, since there are multiple files
involved.

Broadly, what you're trying to accomplish here is to have something
that is crash-safe but without relying on WAL, so that it can work on
standbys. If making things crash-safe without WAL were easy to do, we
probably wouldn't have WAL at all, so it stands to reason that there
are going to be some difficulties here. Making it work reliably is
going to require either inventing some special-purpose type of
write-ahead logging specific to this particular need, or some analogue
of shadow paging, or making sure that every intermediate step is
well-defined and recoverable. Right now, you're on that last path,
and it's by no means obvious to me that that's the wrong place to be,
but I think there's some work left to be done to get it there.

Calling a slot "old" or "new" looks liable to cause problems. Maybe
change those names to contain a character not allowed in a slot name,
if we're going to keep doing it that way.

I wonder if it wouldn't be better to get rid of the subdirectories for
the individual slots, and just have a file pg_replslot/$SLOTNAME, or
not. I know there are later patches that need subdirectories for
their own private data, but they could just create
pg_replslot/$SLOTNAME.dir and put whatever in it they like, without
really implicating this code that much. The advantage of that is that
there would be fewer intermediate states. The slot exists if the file
exists, and not if it doesn't. You still need half-alive and
half-dead until the fsync finishes, but you don't need to worry about
tracking both the state of the directory and the state of the file.
On startup we fsync the containing directory and all of the slot files
we find inside it and refuse to start up if that fails, but once
running filesystem failures only prevent changes; they don't kill the
system.

Patch 0004:

I'm not very confident that PROC_IN_LOGICAL_DECODING is the right way
to go here. It seems to me that excluding the xmins of backends with
slots from globalxmin consideration so that we can fold the same xmin
in by some other mechanism is kind of strange. How about letting the
xmins of such backends affect the computation as normal, and then
having one extra xmin that gets folded in that represents the minima
of the xmin of unconnected slots? When a backend with a slot
disconnects, an on_shmem_exit hook must move that value backwards if
it follows MyPgXact->xmin.

That's all for now...

...Robert

> I wonder if it wouldn't be better to get rid of the subdirectories for
> the individual slots, and just have a file pg_replslot/$SLOTNAME, or
> not. I know there are later patches that need subdirectories for
> their own private data, but they could just create
> pg_replslot/$SLOTNAME.dir and put whatever in it they like, without
> really implicating this code that much. The advantage of that is that
> there would be fewer intermediate states. The slot exists if the file
> exists, and not if it doesn't. You still need half-alive and
> half-dead until the fsync finishes, but you don't need to worry about
> tracking both the state of the directory and the state of the file.

Why do we need directories at all? I know there might be subsidiary
files to store stuff in separate files, but maybe we can just name files
using the slot name (or a transformation thereof) as a prefix. It
shouldn't be difficult to remove the right files whenever there's a
need, and not having to worry about a directory that might need a
separate fsync might make things easier.

On the other hand, there might still be a need to fsync the parent
directory, so maybe there is not that much gain.

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

Hi,

On 2014-01-23 16:04:10 -0500, Robert Haas wrote:
> Patch 0001:
>
> + errmsg("could not find free replication slot"),
>
> Suggest: all replication slots are in use

That sounds better indeed.

> + elog(ERROR, "cannot aquire a slot while another slot
> has been acquired");
>
> Suggest: this backend has already acquired a replication slot
>
> Or demote it to Assert(). I'm not really sure why this needs to be
> checked in non-assert builds.

Hm. Fine with me, not sure why I went with an elog(). Maybe because I
thought output plugin authors could have the idea of using another slot
while inside one?

> I also wonder if we should use the
> terminology "attach" instead of "acquire"; that pairs more naturally
> with "release". Then the message, if we want more than an assert,
> might be "this backend is already attached to a replication slot".

I went with Acquire/Release because our locking code does so, and it
seemed sensible to be consistent. I don't have strong feelings about it.

> + if (slot == NULL)
> + {
> + LWLockRelease(ReplicationSlotCtlLock);
> + ereport(ERROR,
> + (errcode(ERRCODE_UNDEFINED_OBJECT),
> + errmsg("could not find replication
> slot \"%s\"", name)));
> + }
>
> The error will release the LWLock anyway; I'd get rid of the manual
> LWLockRelease, and the braces. Similarly in ReplicationSlotDrop.

Unfortunately not. Inside the walsender there's currently no
LWLockReleaseAll() for ERRORs since commands aren't run inside a
transaction command...

But maybe I should have fixed this by adding the release to
WalSndErrorCleanup() instead? That'd still leave the problematic case
that currently we try to delete a replication slot inside a CATCH when
we fail while initializing the rest of logical replication... But I
guess adding it would be a good idea independent of that.

We could also do a StartTransactionCommand() but I'd rather not, that
currently prevents code in that vicinity from doing anything it
shouldn't via various Assert()s in existing code.

> + /* acquire spinlock so we can test and set ->active safely */
> + SpinLockAcquire(&slot->mutex);
> +
> + if (slot->active)
> + {
> + SpinLockRelease(&slot->mutex);
> + LWLockRelease(ReplicationSlotCtlLock);
> + ereport(ERROR,
> + (errcode(ERRCODE_OBJECT_IN_USE),
> + errmsg("slot \"%s\" already active", name)));
> + }
> +
> + /* we definitely have the slot, no errors possible anymore */
> + slot->active = true;
> + MyReplicationSlot = slot;
> + SpinLockRelease(&slot->mutex);
>
> This doesn't need the LWLockRelease either. It does need the
> SpinLockRelease, but as I think I noted previously, the right way to
> write this is probably: SpinLockAcquire(&slot->mutex); was_active =
> slot->active; slot->active = true; SpinLockRelease(&slot->mutex); if
> (was_active) ereport(). MyReplicatonSlot = slot.

That's not really simpler tho? But if you prefer I can go that way.

> ReplicationSlotsComputeRequiredXmin still acquires ProcArrayLock, and
> the comment "Provide interlock against concurrent recomputations"
> doesn't seem adequate to me. I guess the idea here is that we regard
> ProcArrayLock as protecting ReplicationSlotCtl->catalog_xmin and
> ReplicationSlotCtl->data_xmin, but if that's the idea then we only
> need to hold the lock during the time when we actually update those
> values, not the loop where we compute them.

There's a comment someplace else to that end, but yes, that's
essentially the idea. I decided to take it during the whole
recomputation because we also take ProcArrayLock when creating a new
decoding slot and initially setting ->catalog_xmin. That's not strictly required
but seemed simpler that way, and the code shouldn't be very hot.
The code that initially computes the starting value for catalog_xmin
when creating a new decoding slot has to take ProcArrayLock to be safe,
that's why I though it'd be convenient to always use it for those
values.

In all other cases where we modify *_xmin we're only increasing it which
doesn't need a lock (HS feedback never has taken one, and
GetSnapshotData() modifies ->xmin while holding a shared lock), the only
potential danger is a slight delay in increasing the overall value.

> Also, if that's the
> design, maybe they should be part of PROC_HDR *ProcGlobal rather than
> here. It seems weird to have some of the values protected by
> ProcArrayLock live in a completely different data structure managed
> almost entirely by some other part of the system.

Don't we already have cases of that? I seem to remember so. If you
prefer having them there, I am certainly fine with doing that. This way
they aren't allocated if slots are disabled but it's just two
TransactionIds.

> It's pretty evident that what's currently patch #4 (only peg the xmin
> horizon for catalog tables during logical decoding) needs to become
> patch #1, because it doesn't make any sense to apply this before we do
> that.

Well, the slot code and the the slot support for streaming rep are
independent from and don't use it. So they easily can come before it.

I previously had argued for committing that patch together with the main
changeset extraction commit but you, understandably so!, wanted to have
it separately for review.

> [ discussion about crash safety of slots and their use of PANIC ]
> Broadly, what you're trying to accomplish here is to have something
> that is crash-safe but without relying on WAL, so that it can work on
> standbys. If making things crash-safe without WAL were easy to do, we
> probably wouldn't have WAL at all, so it stands to reason that there
> are going to be some difficulties here.

My big problem here is that you're asking this code to have *higher*
guarantees than WAL ever had and currently has, not equivalent
guarantees. Even though the likelihood of hitting problems is a least a
magnitude or two smaller as we are dealing with minimal amounts of data.
All the situations that seem halfway workable in the nearby thread about
PANIC in XLogInsert() you reference are rough ideas that reduce the
likelihood of PANICs, not remove them.

I am fine with reworking things so that the first operation of several
doesn't PANIC because we still can clearly ERROR out in that case. That
should press the likelihood of problems into the utterly irrelevant
area. E.g. ERROR for the rename(oldpath, newpath) and then start a
critical section for the fsync et al.

> Calling a slot "old" or "new" looks liable to cause problems. Maybe
> change those names to contain a character not allowed in a slot name,
> if we're going to keep doing it that way.

Hm. Fair point. slotname.old, slotname.new sounds better.

> I wonder if it wouldn't be better to get rid of the subdirectories for
> the individual slots, and just have a file pg_replslot/$SLOTNAME, or
> not. I know there are later patches that need subdirectories for
> their own private data, but they could just create
> pg_replslot/$SLOTNAME.dir and put whatever in it they like, without
> really implicating this code that much.

I wondered about making them plain files as well but given the need for
a directory independent from this I don't really see the advantage,
we'll need to handle them anyway during cleanup.

> Patch 0004:
>
> I'm not very confident that PROC_IN_LOGICAL_DECODING is the right way
> to go here. It seems to me that excluding the xmins of backends with
> slots from globalxmin consideration so that we can fold the same xmin
> in by some other mechanism is kind of strange.

It's essentially copying what PROC_IN_VACUUM already does, that's where
I got the idea from.

> How about letting the xmins of such backends affect the computation as normal, and then
> having one extra xmin that gets folded in that represents the minima
> of the xmin of unconnected slots?

That's how I had it in the beginning but it turned out that has
noticeable performance/space impact. Surprising isn't it? The reason is
that we'll intermittently use normal snapshots to look at the catalog
during decoding and they will install a xmin the current proc. So, while
that snapshot is active GetSnapshotData() will return an older xmin
preventing HOT pruning from being as efficient.

I think we *really* need to make heap_page_prune() more efficient CPU
wise someday not too far away.

Greetings,

Andres Freund

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

On 2014-01-24 00:32:09 +0100, Andres Freund wrote:
> I am fine with reworking things so that the first operation of several
> doesn't PANIC because we still can clearly ERROR out in that case. That
> should press the likelihood of problems into the utterly irrelevant
> area. E.g. ERROR for the rename(oldpath, newpath) and then start a
> critical section for the fsync et al.

So, I've changed stuff around to PANIC only as soon as we're in a state
that's unclear.
To test stuff I've added another .so to the test_decoding contrib that
exposes mkdir/rmdir/chmod/unlink to sql to test those cases in the
contrib's sql/slot.sql. Not sure if we want to keep that, but it's
certainly helpful for now.
The required changes certainly didn't make things look nicer...

I've also changed the temporary name used when creating/dropping slots
to $slotname.tmp.

That doesn't remove PANICs but makes them even less likely. Ok?

Greetings,

Andres Freund

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

On Thu, Jan 23, 2014 at 6:32 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I also wonder if we should use the
>> terminology "attach" instead of "acquire"; that pairs more naturally
>> with "release". Then the message, if we want more than an assert,
>> might be "this backend is already attached to a replication slot".
>
> I went with Acquire/Release because our locking code does so, and it
> seemed sensible to be consistent. I don't have strong feelings about it.

Yeah, but I think a slot is not really the same thing as a lock.
Acquire/release might be OK. In some of my recent code I used
attach/detach, which feels a little more natural to me for something
like this, so I lean that direction.

> Unfortunately not. Inside the walsender there's currently no
> LWLockReleaseAll() for ERRORs since commands aren't run inside a
> transaction command...
>
> But maybe I should have fixed this by adding the release to
> WalSndErrorCleanup() instead? That'd still leave the problematic case
> that currently we try to delete a replication slot inside a CATCH when
> we fail while initializing the rest of logical replication... But I
> guess adding it would be a good idea independent of that.

+1. I think that if we can't rely on error handling to clean up the
same things everywhere, it's gonna be a mess. People won't be able to
keep track of which error cleanup is engaged in which code paths, and
screw-ups will result when old code paths are called from new call
sites.

> We could also do a StartTransactionCommand() but I'd rather not, that
> currently prevents code in that vicinity from doing anything it
> shouldn't via various Assert()s in existing code.

Like what? I mean, I'm OK with having a partial error-handling
environment if that's all we need, but I think it's a bad plan to the
extent that the code here needs to be aware of error-handling
differences versus expectations elsewhere in our code.

>> This doesn't need the LWLockRelease either. It does need the
>> SpinLockRelease, but as I think I noted previously, the right way to
>> write this is probably: SpinLockAcquire(&slot->mutex); was_active =
>> slot->active; slot->active = true; SpinLockRelease(&slot->mutex); if
>> (was_active) ereport(). MyReplicatonSlot = slot.
>
> That's not really simpler tho? But if you prefer I can go that way.

It avoids a branch while holding the lock, and it puts the
SpinLockAcquire/Release pair much closer together, so it's easier to
visually verify that the lock is released in all cases.

>> ReplicationSlotsComputeRequiredXmin still acquires ProcArrayLock, and
>> the comment "Provide interlock against concurrent recomputations"
>> doesn't seem adequate to me. I guess the idea here is that we regard
>> ProcArrayLock as protecting ReplicationSlotCtl->catalog_xmin and
>> ReplicationSlotCtl->data_xmin, but if that's the idea then we only
>> need to hold the lock during the time when we actually update those
>> values, not the loop where we compute them.
>
> There's a comment someplace else to that end, but yes, that's
> essentially the idea. I decided to take it during the whole
> recomputation because we also take ProcArrayLock when creating a new
> decoding slot and initially setting ->catalog_xmin. That's not strictly required
> but seemed simpler that way, and the code shouldn't be very hot.
> The code that initially computes the starting value for catalog_xmin
> when creating a new decoding slot has to take ProcArrayLock to be safe,
> that's why I though it'd be convenient to always use it for those
> values.

I don't really see why it's simpler that way. It's clearer what the
point of the lock is if you only hold it for the operations that need
to be protected by that lock.

> In all other cases where we modify *_xmin we're only increasing it which
> doesn't need a lock (HS feedback never has taken one, and
> GetSnapshotData() modifies ->xmin while holding a shared lock), the only
> potential danger is a slight delay in increasing the overall value.

Right. We might want to comment such places.

>> Also, if that's the
>> design, maybe they should be part of PROC_HDR *ProcGlobal rather than
>> here. It seems weird to have some of the values protected by
>> ProcArrayLock live in a completely different data structure managed
>> almost entirely by some other part of the system.
>
> Don't we already have cases of that? I seem to remember so. If you
> prefer having them there, I am certainly fine with doing that. This way
> they aren't allocated if slots are disabled but it's just two
> TransactionIds.

Let's go for it, unless we think of a reason not to.

>> It's pretty evident that what's currently patch #4 (only peg the xmin
>> horizon for catalog tables during logical decoding) needs to become
>> patch #1, because it doesn't make any sense to apply this before we do
>> that.
>
> Well, the slot code and the the slot support for streaming rep are
> independent from and don't use it. So they easily can come before it.

But this code is riddled with places where you track a catalog xmin
and a data xmin separately. The only point of doing it that way is to
support a division that hasn't been made yet.

>> [ discussion about crash safety of slots and their use of PANIC ]
>> Broadly, what you're trying to accomplish here is to have something
>> that is crash-safe but without relying on WAL, so that it can work on
>> standbys. If making things crash-safe without WAL were easy to do, we
>> probably wouldn't have WAL at all, so it stands to reason that there
>> are going to be some difficulties here.
>
> My big problem here is that you're asking this code to have *higher*
> guarantees than WAL ever had and currently has, not equivalent
> guarantees. Even though the likelihood of hitting problems is a least a
> magnitude or two smaller as we are dealing with minimal amounts of data.
> All the situations that seem halfway workable in the nearby thread about
> PANIC in XLogInsert() you reference are rough ideas that reduce the
> likelihood of PANICs, not remove them.
>
> I am fine with reworking things so that the first operation of several
> doesn't PANIC because we still can clearly ERROR out in that case. That
> should press the likelihood of problems into the utterly irrelevant
> area. E.g. ERROR for the rename(oldpath, newpath) and then start a
> critical section for the fsync et al.

I have zero confidence that it's OK to treat fsync() as an operation
that won't fail. Linux documents EIO as a plausible error return, for
example. (And really, how could it not?)

>> Calling a slot "old" or "new" looks liable to cause problems. Maybe
>> change those names to contain a character not allowed in a slot name,
>> if we're going to keep doing it that way.
> I wondered about making them plain files as well but given the need for
> a directory independent from this I don't really see the advantage,
> we'll need to handle them anyway during cleanup.

Yeah, sure, but if it makes for fewer in-between states, it might be worth it.

>> How about letting the xmins of such backends affect the computation as normal, and then
>> having one extra xmin that gets folded in that represents the minima
>> of the xmin of unconnected slots?
>
> That's how I had it in the beginning but it turned out that has
> noticeable performance/space impact. Surprising isn't it? The reason is
> that we'll intermittently use normal snapshots to look at the catalog
> during decoding and they will install a xmin the current proc. So, while
> that snapshot is active GetSnapshotData() will return an older xmin
> preventing HOT pruning from being as efficient.

Hrm, so you still need the flag, to indicate whether the xmin should
be included when we're computing a globalxmin for pruning of a
non-catalog table. But that doesn't necessarily mean that the value
has to live in the slot rather than the PGXACT, does it? It might be
for the best the way you have it, but it does look kind of weird. Not
sure yet.

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

On 2014-01-24 12:10:50 -0500, Robert Haas wrote:
> > Unfortunately not. Inside the walsender there's currently no
> > LWLockReleaseAll() for ERRORs since commands aren't run inside a
> > transaction command...
> >
> > But maybe I should have fixed this by adding the release to
> > WalSndErrorCleanup() instead? That'd still leave the problematic case
> > that currently we try to delete a replication slot inside a CATCH when
> > we fail while initializing the rest of logical replication... But I
> > guess adding it would be a good idea independent of that.
>
> +1. I think that if we can't rely on error handling to clean up the
> same things everywhere, it's gonna be a mess. People won't be able to
> keep track of which error cleanup is engaged in which code paths, and
> screw-ups will result when old code paths are called from new call
> sites.

Ok, I'll additionally add it there.

> > We could also do a StartTransactionCommand() but I'd rather not, that
> > currently prevents code in that vicinity from doing anything it
> > shouldn't via various Assert()s in existing code.
>
> Like what? I mean, I'm OK with having a partial error-handling
> environment if that's all we need, but I think it's a bad plan to the
> extent that the code here needs to be aware of error-handling
> differences versus expectations elsewhere in our code.

Catalog lookups, building a snapshot, xid assignment, whatnot. All that
shouldn't happen in the locations creating/dropping a slot.
I think we should at some point separate parts of the error handling out
of xact.c. Currently its repeated slightly differently over logs of
places (check e.g. the callsites for LWLockReleaseAll), that's not
robust. But that's a project for another day.

Note that the actual decoding *does* happen inside a TransactionCommand,
as it'd be failure prone to copy all the cleanup logic. And we need to
have most of the normal cleanup code.

> I don't really see why it's simpler that way. It's clearer what the
> point of the lock is if you only hold it for the operations that need
> to be protected by that lock.

> > In all other cases where we modify *_xmin we're only increasing it which
> > doesn't need a lock (HS feedback never has taken one, and
> > GetSnapshotData() modifies ->xmin while holding a shared lock), the only
> > potential danger is a slight delay in increasing the overall value.

> Right. We might want to comment such places.

> > Don't we already have cases of that? I seem to remember so. If you
> > prefer having them there, I am certainly fine with doing that. This way
> > they aren't allocated if slots are disabled but it's just two
> > TransactionIds.
>
> Let's go for it, unless we think of a reason not to.

ok on those counts.

> >> It's pretty evident that what's currently patch #4 (only peg the xmin
> >> horizon for catalog tables during logical decoding) needs to become
> >> patch #1, because it doesn't make any sense to apply this before we do
> >> that.
> >
> > Well, the slot code and the the slot support for streaming rep are
> > independent from and don't use it. So they easily can come before it.
>
> But this code is riddled with places where you track a catalog xmin
> and a data xmin separately. The only point of doing it that way is to
> support a division that hasn't been made yet.

If you think it will make stuff more manageable I can try separating all
lines dealing with catalog_xmin into another patch as long as data_xmin
doesn't have to be renamed.
That said, I don't really think it's a big problem that the division
hasn't been made, essentially the meaning is different, even if we don't
take advantage of it yet. data_xmin is there for streaming replication
where we need to prevent all removal, catalog_xmin is there for
changeset extraction.

> I have zero confidence that it's OK to treat fsync() as an operation
> that won't fail. Linux documents EIO as a plausible error return, for
> example. (And really, how could it not?)

But quite fundamentally having a the most basic persistency building
block fail is something you can't really handle safely. Note that
issue_xlog_fsync() has always (and I wager, will always) treated that as
a PANIC.
I don't recall many complaints about that for WAL. All of the ones I
found in a quick search were like "oh, the fs invalidated my fd because
of corruption". And few.

> >> Calling a slot "old" or "new" looks liable to cause problems. Maybe
> >> change those names to contain a character not allowed in a slot name,
> >> if we're going to keep doing it that way.
> > I wondered about making them plain files as well but given the need for
> > a directory independent from this I don't really see the advantage,
> > we'll need to handle them anyway during cleanup.
>
> Yeah, sure, but if it makes for fewer in-between states, it might be worth it.

I don't think it'd make anything simpler with the new version of the
code. Agreed?

Greetings,

Andres Freund

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

On Fri, Jan 24, 2014 at 12:49 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> But this code is riddled with places where you track a catalog xmin
>> and a data xmin separately. The only point of doing it that way is to
>> support a division that hasn't been made yet.
>
> If you think it will make stuff more manageable I can try separating all
> lines dealing with catalog_xmin into another patch as long as data_xmin
> doesn't have to be renamed.
> That said, I don't really think it's a big problem that the division
> hasn't been made, essentially the meaning is different, even if we don't
> take advantage of it yet. data_xmin is there for streaming replication
> where we need to prevent all removal, catalog_xmin is there for
> changeset extraction.

I spent some more time studying the 0001 and 0002 patches this
afternoon, with a side dish of 0004. I'm leaning toward thinking we
should go ahead and make that division. I'm also wondering about
whether we've got the right naming here. AFAICT, it's not the case
that we're going to use the "catalog xmin" for catalogs and the "data
xmin" for non-catalogs. Rather, the "catalog xmin" is going to always
be included in globalxmin calculations, so IOW it should just be
called "xmin". The "data xmin" is going to be included only for
non-catalog tables. I guess "data" is a reasonable antonym for
catalog, but I'm slightly tempted to propose
RecentGlobalNonCatalogXmin and similar. Maybe that's too ugly to
live, but I can see someone failing to guess what the exact
distinction is between "xmin" and "data xmin", and I bet they'd be a
lot less likely to misguess if we wrote "non catalog".

It's interesting (though not immediately relevant) to speculate about
how we might extend this to fine-grained xmin tracking more generally.
The design sketch that comes to mind (and I think parts of this have
been proposed before) is to have a means by which backends can promise
not to lock any more tables except under a new snapshot. At the read
committed isolation level, or in any single-statement transaction,
backends can so promise whenever (a) all tables mentioned in the query
have been locked and (b) all functions to be invoked during the query
via the fmgr interface promise (e.g. via function labeling) that they
won't directly or indirectly do such a thing. If they break their
promise, we detect it and ereport(ERROR). Backends that have made
such a guarantee can be ignored for global-xmin calculations that
don't involve the tables they have locked. One idea is to keep a hash
table keyed by <dboid, reloid> with some limited number of entries in
shared memory; it caches the table-specific xmin, a usage counter, and
a flag indicating whether the cached xmin might be stale. In order to
promise not to lock any new tables, backends must make or update
entries for all the tables they already have locked in this hash
table; if there aren't enough entries, they're not allowed to promise.
Thus, backends wishing to prune can use the cached xmin value if it's
present (optionally updating it if it's stale) and the minimum of the
xmins of the backends that haven't made a promise if it isn't. This
is a bit hairy though; access to the shared hash table had better be
*really* fast, and we'd better not need to recompute the cached value
too often.

Anyway, whether we end up pursuing something like that or not, I think
I'm persuaded that this particular optimization won't really be a
problem for hypothetical future work in this area; and also that it's
a good idea to do this much now specifically for logical decoding.

>> I have zero confidence that it's OK to treat fsync() as an operation
>> that won't fail. Linux documents EIO as a plausible error return, for
>> example. (And really, how could it not?)
>
> But quite fundamentally having a the most basic persistency building
> block fail is something you can't really handle safely. Note that
> issue_xlog_fsync() has always (and I wager, will always) treated that as
> a PANIC.
> I don't recall many complaints about that for WAL. All of the ones I
> found in a quick search were like "oh, the fs invalidated my fd because
> of corruption". And few.

Well, you have a point. And certainly this version looks much better
than the previous version in terms of the likelihood of PANIC
occurring in practice. But I wonder if we couldn't cut it down even
further without too much effort. Suppose we define a slot to exist
if, and only if, the state file exists. A directory without a state
file is subject to arbitrary removal. Then we can proceed as follows:

- mkdir() the directory.
- open() state.tmp
- write() state.tmp
- close() state.tmp
- fsync() parent directory, directory and state.tmp
- rename() state.tmp to state
- fsync() state

If any step except the last one fails, no problem. The next startup
can nuke the leftovers; any future attempt to create a slot with the
name can ignore an EEXIST failure from mkdir() and procedure just as
above. Only a failure of the very last fsync is a PANIC. In some
ways I think this'd be simpler than what you've got now, because we'd
eliminate the dance with renaming the directory as well as the state
file; only the state file matters.

To drop a slot, just unlink the state file and fsync() the directory.
If the unlink fails, it's just an error. If the fsync() fails, it's a
PANIC. Once the state file is gone, removing everything else is only
an ERROR, and you don't even need to fsync() it again.

To update a slot, open, write, close, and fsync state.tmp, then rename
it to state and fsync() again. None of these steps need PANIC; hold
off on updating the values in memory until they're all done. If any
step fails, the attempt to update the slot fails, but either memory
and disk are still consistent, or the disk has an xmin newer than
memory, but still legal. On restart, when restoring slots, fsync()
each state file, dying horribly if we can't, and remove any
directories that don't contain one.

As compared with what you have here, this eliminates the risk of PANIC
entirely for slot updates, which is good because those will be quite
frequent. For creating or dropping a slot, it doesn't quite eliminate
the risk entirely but only one fsync() call per create or drop is at
risk. We still risk startup time failures, but that's unavoidable
anyway if the data we need can't be read; the chances of blowing up a
running system are very low.

Looking over patch 0002, I see that there's code to allow a walsender
to create or drop a physical replication slot. Also, if we've
acquired a replication slot, there's code to update it, and code to
make sure we disconnect from it, but there's no code to acquire it. I
think maybe the hunk in StartReplication() is supposed to be calling
ReplicationSlotAcquire() instead of ReplicationSlotRelease(), which
(ahem) makes one wonder how thoroughly this code has been tested.
There's also no provision for walsender (or pg_receivexlog?) to send
the new SLOT option to walreceiver, which seems somewhat necessary.
I'm tempted to suggest also adding something to src/bin/scripts to
create and drop slots, though I suppose we could just recommend psql
-c 'CREATE_REPLICATION_SLOT SLOT zippy PHYSICAL' 'replication=true'.

(BTW, isn't that kind of a strange syntax, with the word SLOT
appearing twice? I think we could drop the second one.)

It also occurred to me that we need documentation for all of this; I
see that's in patch 0010, but I haven't reviewed it in detail yet.

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

Hi Robert, all,

On 2014-01-24 20:38:11 -0500, Robert Haas wrote:
> On Fri, Jan 24, 2014 at 12:49 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> >> But this code is riddled with places where you track a catalog xmin
> >> and a data xmin separately. The only point of doing it that way is to
> >> support a division that hasn't been made yet.
> >
> > If you think it will make stuff more manageable I can try separating all
> > lines dealing with catalog_xmin into another patch as long as data_xmin
> > doesn't have to be renamed.
> > That said, I don't really think it's a big problem that the division
> > hasn't been made, essentially the meaning is different, even if we don't
> > take advantage of it yet. data_xmin is there for streaming replication
> > where we need to prevent all removal, catalog_xmin is there for
> > changeset extraction.
>
> I spent some more time studying the 0001 and 0002 patches this
> afternoon, with a side dish of 0004. I'm leaning toward thinking we
> should go ahead and make that division.

Ok.

> I'm also wondering about
> whether we've got the right naming here. AFAICT, it's not the case
> that we're going to use the "catalog xmin" for catalogs and the "data
> xmin" for non-catalogs. Rather, the "catalog xmin" is going to always
> be included in globalxmin calculations, so IOW it should just be
> called "xmin".

Well, not really. That's true for GetSnapshotData(), but not for
GetOldestXmin() where we calculate an xmin *not* including the catalog
xmin. And the data_xmin is always used, regardless of
catalog/non_catalog, we peg the xmin further for catalog tables, based
on that value.
The reason for doing things this way is that it makes all current usages
of RecentGlobalXmin safe, since that is the more conservative
value. Only in inspected location we can use RecentGlobalDataXmin which
*does* include data_xmin but *not* catalog_xmin.

> It's interesting (though not immediately relevant) to speculate about
> how we might extend this to fine-grained xmin tracking more generally.
> [musings for another time]

Yea, I have wondered about that as well. I think the easiest thing would
be to to compute RecentGlobalDataXmin in a database specific manner
since by definition it will *not* include shared tables. We do that
already for GetOldestXmin() but that's not used for heap pruning. I'd
quickly tested that some months back and it gave significant speedups
for two pgbenches in two databases.

> >> I have zero confidence that it's OK to treat fsync() as an operation
> >> that won't fail. Linux documents EIO as a plausible error return, for
> >> example. (And really, how could it not?)
> >
> > But quite fundamentally having a the most basic persistency building
> > block fail is something you can't really handle safely. Note that
> > issue_xlog_fsync() has always (and I wager, will always) treated that as
> > a PANIC.
> > I don't recall many complaints about that for WAL. All of the ones I
> > found in a quick search were like "oh, the fs invalidated my fd because
> > of corruption". And few.
>
> Well, you have a point. And certainly this version looks much better
> than the previous version in terms of the likelihood of PANIC
> occurring in practice. But I wonder if we couldn't cut it down even
> further without too much effort. Suppose we define a slot to exist
> if, and only if, the state file exists. A directory without a state
> file is subject to arbitrary removal. Then we can proceed as follows:
>
> - mkdir() the directory.
> - open() state.tmp
> - write() state.tmp
> - close() state.tmp
> - fsync() parent directory, directory and state.tmp
> - rename() state.tmp to state
> - fsync() state
>
> If any step except the last one fails, no problem. The next startup
> can nuke the leftovers; any future attempt to create a slot with the
> name can ignore an EEXIST failure from mkdir() and procedure just as
> above. Only a failure of the very last fsync is a PANIC. In some
> ways I think this'd be simpler than what you've got now, because we'd
> eliminate the dance with renaming the directory as well as the state
> file; only the state file matters.

Hm. I think this is pretty exactly what happens in the current patch,
right? There's an additional fsync() of the parent directory at the end,
but that's it.

> To drop a slot, just unlink the state file and fsync() the directory.
> If the unlink fails, it's just an error. If the fsync() fails, it's a
> PANIC. Once the state file is gone, removing everything else is only
> an ERROR, and you don't even need to fsync() it again.

Well, the patch as is renames the directory first and fsyncs that. Only
a failure in fsyncing is punishable by PANIC, if rmtree() on the temp
directory file fails it generates WARNINGs, that's it.

> To update a slot, open, write, close, and fsync state.tmp, then rename
> it to state and fsync() again. None of these steps need PANIC; hold
> off on updating the values in memory until they're all done. If any
> step fails, the attempt to update the slot fails, but either memory
> and disk are still consistent, or the disk has an xmin newer than
> memory, but still legal. On restart, when restoring slots, fsync()
> each state file, dying horribly if we can't, and remove any
> directories that don't contain one.

That's again pretty similar to what happens, only that we panic if the
fsync()ing fails. And I think that's correct.

I still think worrying over this to this degree is a waste of
effort. There's much hotter places that could be inspected to that
detail than this.

> Looking over patch 0002, I see that there's code to allow a walsender
> to create or drop a physical replication slot. Also, if we've
> acquired a replication slot, there's code to update it, and code to
> make sure we disconnect from it, but there's no code to acquire it. I
> think maybe the hunk in StartReplication() is supposed to be calling
> ReplicationSlotAcquire() instead of ReplicationSlotRelease(),

Uh. You had me worried here for a minute or two, a hunk or two earlier
than the ReplicationSlotRelease() you mention. What probably confused
you is that StartReplication only returns once all streaming is
finished. Not my idea...

static void
StartReplication(StartReplicationCmd *cmd)
{
...
if (cmd->slotname)
ReplicationSlotAcquire(cmd->slotname);
...
...
/* this is where we'll actually loop busily */
WalSndLoop(XLogSendPhysical);
...
if (cmd->slotname)
ReplicationSlotRelease();
...
}

> which
> (ahem) makes one wonder how thoroughly this code has been tested.

It's actually tested as of a week ago or so. Both with pg_receivexlog
and a hacked up walreceiver. That's how I noticed
a472ae1e4e2bf5fb71ac655d38d1e35df4c1c966 ;). Because it did *not* work
properly in the beginning... But it didn't end up being my code. Hah!

> There's also no provision for walsender (or pg_receivexlog?) to send
> the new SLOT option to walreceiver, which seems somewhat necessary.

There's a hacked up pg_receivexlog in the last commit in the series. I
haven't included the hack for walreceiver as it was too embarassing for
the public eye.

I really, really don't want to focus on polishing up the receiver side
for this before the basics of changeset extraction are done. I've very,
very reluctantly agreed to generalize the slot concept for streaming rep
now, but I said all along that I won't do the client work till the
changeset extraction stuff is done. There is a good deal of UI design
work to be done, and I don't think I have the capacity to tackle that
right now.

> I'm tempted to suggest also adding something to src/bin/scripts to
> create and drop slots, though I suppose we could just recommend psql
> -c 'CREATE_REPLICATION_SLOT SLOT zippy PHYSICAL' 'replication=true'.

There's an SQL function for doing so. In, err, the wrong patch:
postgres=# \df create_physical_replication_slot
List of functions
Schema | Name | Result data type | Argument data types | Type
------------+----------------------------------+------------------+----------------------------------------------------------+--------
pg_catalog | create_physical_replication_slot | record | slotname name, OUT slotname text, OUT xlog_position text | normal
(1 row)

will move it. Not sure if we need the slotname as an OUT value as well,
it's helpful as part of the additional return types for creating a
decoding slot, but here...

Not sure if there's still a reason for a separate commandline utility?

> (BTW, isn't that kind of a strange syntax, with the word SLOT
> appearing twice? I think we could drop the second one.)

Well, that's what I'd suggested on the mailinglist, so I didn't change
it. It will definitely be a separate SLOT slot_name for
START_REPLICATION, that's pretty much the only reason for keeping it
separate for CREATE/DROP. Don't care which way we go in the end.

> It also occurred to me that we need documentation for all of this; I
> see that's in patch 0010, but I haven't reviewed it in detail yet.

The streaming rep part is scantily documented since that's pending the
clientside work, but the changeset extraction part should be documented
to some degree... Craig worked on my initial docs and seemed to be able
to make enough sense of it, so I hope it's not in a totally bad state.

Greetings,

Andres Freund

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

On Sat, Jan 25, 2014 at 5:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Looking over patch 0002, I see that there's code to allow a walsender
>> to create or drop a physical replication slot. Also, if we've
>> acquired a replication slot, there's code to update it, and code to
>> make sure we disconnect from it, but there's no code to acquire it. I
>> think maybe the hunk in StartReplication() is supposed to be calling
>> ReplicationSlotAcquire() instead of ReplicationSlotRelease(),
>
> Uh. You had me worried here for a minute or two, a hunk or two earlier
> than the ReplicationSlotRelease() you mention. What probably confused
> you is that StartReplication only returns once all streaming is
> finished. Not my idea...

No, what confuses me is that there's no call to
ReplicationSlotAcquire() in patch 0001 or patch 0002.... the function
is added but not called.

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

On Sat, Jan 25, 2014 at 5:25 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> I'm also wondering about
>> whether we've got the right naming here. AFAICT, it's not the case
>> that we're going to use the "catalog xmin" for catalogs and the "data
>> xmin" for non-catalogs. Rather, the "catalog xmin" is going to always
>> be included in globalxmin calculations, so IOW it should just be
>> called "xmin".
>
> Well, not really. That's true for GetSnapshotData(), but not for
> GetOldestXmin() where we calculate an xmin *not* including the catalog
> xmin. And the data_xmin is always used, regardless of
> catalog/non_catalog, we peg the xmin further for catalog tables, based
> on that value.
> The reason for doing things this way is that it makes all current usages
> of RecentGlobalXmin safe, since that is the more conservative
> value. Only in inspected location we can use RecentGlobalDataXmin which
> *does* include data_xmin but *not* catalog_xmin.

Well, OK, so I guess I'm turned around. But I guess my point is - if
one of data_xmin and catalog_xmin is really just xmin, then I think it
would be more clear to call that one "xmin".

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

Hi,

Here's the next version of the patchset. The following changes have been
made:
* move xmin pegging and more logic responsibility to procarray.c
* split all support for changeset extraction from the initial slot patch
* always register an before_shmem_exit handler when
max_replication_slots is registered, not just while a slot is acquired
* move some patch hunks to earlier patches, especially the
ReplicationSlotAcquire() call for physical rep that accidentally
slipped and the SQL accessible slot manipulation functions
* minor stuff

Greetings,

Andres Freund

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

On 27 January 2014 16:20, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> Here's the next version of the patchset. The following changes have been
> made:
> * move xmin pegging and more logic responsibility to procarray.c
> * split all support for changeset extraction from the initial slot patch
> * always register an before_shmem_exit handler when
> max_replication_slots is registered, not just while a slot is acquired
> * move some patch hunks to earlier patches, especially the
> ReplicationSlotAcquire() call for physical rep that accidentally
> slipped and the SQL accessible slot manipulation functions
> * minor stuff

0001 doesn't apply cleanly due to commit
ea9df812d8502fff74e7bc37d61bdc7d66d77a7f.

The rest are fine.

--
Thom

On Tue, Jan 28, 2014 at 11:49 AM, Thom Brown <thom(at)linux(dot)com> wrote:
> On 27 January 2014 16:20, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Hi,
>>
>> Here's the next version of the patchset. The following changes have been
>> made:
>> * move xmin pegging and more logic responsibility to procarray.c
>> * split all support for changeset extraction from the initial slot patch
>> * always register an before_shmem_exit handler when
>> max_replication_slots is registered, not just while a slot is acquired
>> * move some patch hunks to earlier patches, especially the
>> ReplicationSlotAcquire() call for physical rep that accidentally
>> slipped and the SQL accessible slot manipulation functions
>> * minor stuff
>
> 0001 doesn't apply cleanly due to commit
> ea9df812d8502fff74e7bc37d61bdc7d66d77a7f.
>
> The rest are fine.

I've rebased it here and am hacking on it still.

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

On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I've rebased it here and am hacking on it still.

Andres and I are going back and forth between our respective git repos
hacking on this, and I think we're getting there, but I have a
terminological question which I'd like to submit to a wider audience:

The point of Andres's patch set is to introduce a new technology
called logical decoding; that is, the ability to get a replication
stream that is based on changes to tuples rather than changes to
blocks. It could also be called logical replication. In these
patches, our existing replication is referred to as "physical"
replication, which sounds kind of funny to me. Anyone have another
suggestion?

There are a lot of ways to slice the space of possible replication
solutions. We currently talk about "streaming replication" (as
opposed to "archiving") and "synchronous replication" (as opposed to
asynchronous), but this is a new distinction. At least in theory,
whether replication is "physical" or logical is independent of whether
it's based on streaming or archiving and also of whether it's
synchronous or asynchronous. So we can't for example talk about
"logical replication" in opposition to "streaming replication"; that's
comparing apples and oranges. We need a pair of new terms, and I
can't immediately think of anything better than physical/logical, but
it still sounds somewhat awkward to me so ... anyone else have an
idea?

Thanks,

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

On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> I've rebased it here and am hacking on it still.
>
> Andres and I are going back and forth between our respective git repos
> hacking on this, and I think we're getting there, but I have a
> terminological question which I'd like to submit to a wider audience:
>
> The point of Andres's patch set is to introduce a new technology
> called logical decoding; that is, the ability to get a replication
> stream that is based on changes to tuples rather than changes to
> blocks. It could also be called logical replication. In these
> patches, our existing replication is referred to as "physical"
> replication, which sounds kind of funny to me. Anyone have another
> suggestion?

Logical and Binary replication?

--
Thom

On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
> On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >> I've rebased it here and am hacking on it still.
> >
> > Andres and I are going back and forth between our respective git repos
> > hacking on this, and I think we're getting there, but I have a
> > terminological question which I'd like to submit to a wider audience:
> >
> > The point of Andres's patch set is to introduce a new technology
> > called logical decoding; that is, the ability to get a replication
> > stream that is based on changes to tuples rather than changes to
> > blocks. It could also be called logical replication. In these
> > patches, our existing replication is referred to as "physical"
> > replication, which sounds kind of funny to me. Anyone have another
> > suggestion?
>
> Logical and Binary replication?

Unfortunately changeset extraction output's can be binary data...

Greetings,

Andres Freund

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

On 28 January 2014 21:56, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
>> On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> > On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>> >> I've rebased it here and am hacking on it still.
>> >
>> > Andres and I are going back and forth between our respective git repos
>> > hacking on this, and I think we're getting there, but I have a
>> > terminological question which I'd like to submit to a wider audience:
>> >
>> > The point of Andres's patch set is to introduce a new technology
>> > called logical decoding; that is, the ability to get a replication
>> > stream that is based on changes to tuples rather than changes to
>> > blocks. It could also be called logical replication. In these
>> > patches, our existing replication is referred to as "physical"
>> > replication, which sounds kind of funny to me. Anyone have another
>> > suggestion?
>>
>> Logical and Binary replication?
>
> Unfortunately changeset extraction output's can be binary data...

"system"?
"cluster"?
"full"?
"complete"?

--
Thom

On Tue, Jan 28, 2014 at 4:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:

> On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
> > On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> > >> I've rebased it here and am hacking on it still.
> > >
> > > Andres and I are going back and forth between our respective git repos
> > > hacking on this, and I think we're getting there, but I have a
> > > terminological question which I'd like to submit to a wider audience:
> > >
> > > The point of Andres's patch set is to introduce a new technology
> > > called logical decoding; that is, the ability to get a replication
> > > stream that is based on changes to tuples rather than changes to
> > > blocks. It could also be called logical replication. In these
> > > patches, our existing replication is referred to as "physical"
> > > replication, which sounds kind of funny to me. Anyone have another
> > > suggestion?
> >
> > Logical and Binary replication?
>
> Unfortunately changeset extraction output's can be binary data...
>

Perhaps Logical and Block?

The existing replication mechanism is similar to block-based disk backups.
It's the whole thing (not parts) and doesn't have any concept of
database/directory.

On Tue, Jan 28, 2014 at 05:31:25PM -0500, Rod Taylor wrote:
> On Tue, Jan 28, 2014 at 4:56 PM, Andres Freund <andres(at)2ndquadrant(dot)com>wrote:
>
> > On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
> > > On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > > > On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> > wrote:
> > > >> I've rebased it here and am hacking on it still.
> > > >
> > > > Andres and I are going back and forth between our respective git repos
> > > > hacking on this, and I think we're getting there, but I have a
> > > > terminological question which I'd like to submit to a wider audience:
> > > >
> > > > The point of Andres's patch set is to introduce a new technology
> > > > called logical decoding; that is, the ability to get a replication
> > > > stream that is based on changes to tuples rather than changes to
> > > > blocks. It could also be called logical replication. In these
> > > > patches, our existing replication is referred to as "physical"
> > > > replication, which sounds kind of funny to me. Anyone have another
> > > > suggestion?
> > >
> > > Logical and Binary replication?
> >
> > Unfortunately changeset extraction output's can be binary data...
> >
>
> Perhaps Logical and Block?
>
> The existing replication mechanism is similar to block-based disk backups.
> It's the whole thing (not parts) and doesn't have any concept of
> database/directory.

+1 for this terminology. It's descriptive.

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 01/28/2014 10:56 PM, Andres Freund wrote:
> On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
>> On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>> The point of Andres's patch set is to introduce a new technology
>>> called logical decoding; that is, the ability to get a replication
>>> stream that is based on changes to tuples rather than changes to
>>> blocks. It could also be called logical replication. In these
>>> patches, our existing replication is referred to as "physical"
>>> replication, which sounds kind of funny to me. Anyone have another
>>> suggestion?
>>
>> Logical and Binary replication?
>
> Unfortunately changeset extraction output's can be binary data...

I think "physical" and "logical" are fine and they seem to be well known
terminology. Oracle uses those words and I have also seen many places
use "physical backup" and "logical backup", for example on Barman's
homepage.

--
Andreas Karlsson

On Tue, Jan 28, 2014 at 7:43 PM, Andreas Karlsson <andreas(at)proxel(dot)se> wrote:
> I think "physical" and "logical" are fine and they seem to be well known
> terminology. Oracle uses those words and I have also seen many places use
> "physical backup" and "logical backup", for example on Barman's homepage.

There's certainly something to be said for this.

Another idea I had this evening was "logical replication" and
"WAL-based replication", but that's a bit confusing too since logical
rep. is going to use WAL as an underlying technology.

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

Andreas Karlsson wrote:
> On 01/28/2014 10:56 PM, Andres Freund wrote:
>> On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
>>> On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
>>>> The point of Andres's patch set is to introduce a new technology
>>>> called logical decoding; that is, the ability to get a replication
>>>> stream that is based on changes to tuples rather than changes to
>>>> blocks. It could also be called logical replication. In these
>>>> patches, our existing replication is referred to as "physical"
>>>> replication, which sounds kind of funny to me. Anyone have another
>>>> suggestion?
>>>
>>> Logical and Binary replication?
>>
>> Unfortunately changeset extraction output's can be binary data...
>
> I think "physical" and "logical" are fine and they seem to be well known
> terminology. Oracle uses those words and I have also seen many places
> use "physical backup" and "logical backup", for example on Barman's
> homepage.

+1

I think it also fits well with the well-known terms "physical [database]
design" and "logical design". Not that it is the same thing, but I
believe that every database person, when faced with the distiction
"physical" versus "logical", will conclude that the former refers to
data placement or block structure, while the latter refers to the
semantics of the data being stored.

Yours,
Laurenz Albe

It seems to me that the terms "physical", "logical", and "binary" are
always relative to the perspective of the component being worked on.

"Physical" often means "one level of abstraction below mine, and upon which
my work builds". "Logical" means "my work's level of abstraction". And
"Binary" means "data which I'm not going to pretend I know or care how to
interpret."

So I'd suggest "block" and "tuple", perhaps.

On Wed, Jan 29, 2014 at 4:25 AM, Albe Laurenz <laurenz(dot)albe(at)wien(dot)gv(dot)at>wrote:

> Andreas Karlsson wrote:
> > On 01/28/2014 10:56 PM, Andres Freund wrote:
> >> On 2014-01-28 21:48:09 +0000, Thom Brown wrote:
> >>> On 28 January 2014 21:37, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> >>>> On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com>
> wrote:
> >>>> The point of Andres's patch set is to introduce a new technology
> >>>> called logical decoding; that is, the ability to get a replication
> >>>> stream that is based on changes to tuples rather than changes to
> >>>> blocks. It could also be called logical replication. In these
> >>>> patches, our existing replication is referred to as "physical"
> >>>> replication, which sounds kind of funny to me. Anyone have another
> >>>> suggestion?
> >>>
> >>> Logical and Binary replication?
> >>
> >> Unfortunately changeset extraction output's can be binary data...
> >
> > I think "physical" and "logical" are fine and they seem to be well known
> > terminology. Oracle uses those words and I have also seen many places
> > use "physical backup" and "logical backup", for example on Barman's
> > homepage.
>
> +1
>
> I think it also fits well with the well-known terms "physical [database]
> design" and "logical design". Not that it is the same thing, but I
> believe that every database person, when faced with the distiction
> "physical" versus "logical", will conclude that the former refers to
> data placement or block structure, while the latter refers to the
> semantics of the data being stored.
>
> Yours,
> Laurenz Albe
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers(at)postgresql(dot)org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers
>

On Tue, Jan 28, 2014 at 11:53 AM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> I've rebased it here and am hacking on it still.

OK. The attached patch is a combination of Andres' first two patches
with lots more changes from both Andres and myself. At this point,
I'm pretty happy with this, and propose to commit the attached
version, absent objection. All by itself, it provides a useful new
option for users, and it sets the stage for the subsequent logical
decoding patches as well. For those not following along closely,
here's the short version: if you choose to create a replication slot,
you can make the master retain the exact amount of WAL that the
standby still needs, rather than guessing what value to set for
wal_keep_segments; also, you can avoid hot standby conflicts even when
the connection between master and slave is interrupted (but the master
will bloat if the interruption is long, so watch out). For logical
decoding, this functionality is essential rather than nice-to-have.

Here's a summary of what we've changed since the last version Andres posted:

- Fairly extensive revisions to slot error handling, eliminating the
PG_TRY/PG_CATCH blocks that were present before (which I didn't
believe would work as designed) and cleaning up some corner cases to
eliminate unnecessary failures.
- Rigid enforcement of existing PG practices around spinlocks,
volatile pointers, and memory barriers.
- Modification of the on-disk format for slots so that we don't
serialize junk that properly only lives in memory.
- Removal of various references to decoding and logical replication
that properly belong in subsequent patches.
- Support for using slots via a new recovery.conf parameter,
primary_slotname, and a new pg_receivexlog option, --slot. I felt
this was important because, without this, you couldn't test that it
actually works without applying the remainder of Andres's patch set,
and even then you could mostly only test logical replication. With
it, this is an independently useful and testable feature.
- Exclude pg_replslot from base backups. This might need more thought
and documentation; people who use the filesystem method to perform
backups might need to be advised to remove this directory in some
cases also, or people who use pg_basebackup might want to keep it in
some cases (not sure).
- Lots of renaming to make the names more clear and consistent.
- Lots of bug fixes, minor tinkering, comment changes, and cleanups.
- Documentation.

For those wishing to see the blow-by-blow:

http://git.postgresql.org/gitweb/?p=users/rhaas/postgres.git;a=shortlog;h=refs/heads/slot2

In the future (i.e. post-9.4), I think we'll likely want to extend
this in a bunch of interesting ways. I strongly suspect people are
going to want to have an option for slots that pin the LSN but not
xmin, and I also think they're going to want slots that hold LSN or
xmin for a certain amount of time after a disconnect but then give up,
or maybe a certain number of segments/transaction IDs and then give
up. Nonwithstanding those important improvements, I think this is a
very credible v1 of this functionality.

Thanks,

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

Hi,

attached you can find the next version of the patchset.

Changes:
* rebased ontop the committed slot patch (Thanks Robert!), that required
a fair amount of work
* adjusted naming of the SQL interface functions, to be consisted with ^
* several patches of the patchseries were merged
* Large amount of comment copy-editing
* Some code restructuring

There's one major things I am not yet really happy with which is the is
the integration of how decoding snapshots are integrated. I've gone back
and forth over it today, but I think I need a decent night of sleep to
bring it to a conclusion...

The patches are currently:

0001: wal_decoding: Introduce logical changeset extraction.
The meat of the functionality, including the SQL interface.

0002: wal_decoding: logical changeset extraction walsender interface
Walsender integration of changeset extraction, including support for
synchronous replication.

0003: wal_decoding: pg_recvlogical: Introduce pg_receivexlog equivalent for logical changes
Simple tool for receiving the changes over the walsender interface.

0004: wal_decoding: Documentation for replication slots and changeset extraction
...

0005: wal_decoding: Temporarily add logical decoding regression tests to everything
This is a patch I don't think should be finally applied, but which is
very helpful during debugging. It simply adds tests to the beginning/end of
the normal regression tests, decoding it in its entirety.

As always it's also pushed to
http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
branch xlog-decoding-rebasing-remapping

Greetings,

Andres Freund

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

On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> 0004: wal_decoding: Documentation for replication slots and changeset extraction

The usage of pg_create_decoding_replication_slot does show the "(1 row)" line.

The output of "SELECT * FROM pg_replication_slots;" is out-of-date.

There appears to be a column named "slot_name" and "slottype". Could
one of these have or not have the underscore for consistency?

The example also shows output from pg_decoding_slot_get_changes after
inserting 2 rows, but when I run the same example, there are no rows
returned:

# BEGIN;
BEGIN

*# INSERT INTO data(data) VALUES('1');
INSERT 0 1

*# INSERT INTO data(data) VALUES('1');
INSERT 0 1

*# COMMIT;
COMMIT

# SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
'include-xids', '0');
location | xid | data
----------+-----+------
(0 rows)

I inserted a single row outside of a transaction, and got the expected
output. Then I ran the above again, and got an output, but an
unexpected one:

SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
'include-xids', '0');
location | xid | data
-----------+-----+-----------------------------------------------
0/16C8B90 | 769 | BEGIN
0/16C8D50 | 769 | table "data": INSERT: id[int4]:3 data[text]:1
0/16C8D50 | 769 | COMMIT
(3 rows)

And running the transaction with inserts again, there's no output from
that same function command. I always get an output from isolated
INSERT statements. I should point out that in my .psqlrc file I have
"\set ON_ERROR_ROLLBACK". If I use psql -X, this symptom no longer
occurs, so I think the automatic savepoints are interfering, and the
effect appears to be inconsistent.

--
Thom

On 7 February 2014 20:58, Thom Brown <thom(at)linux(dot)com> wrote:
> On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> 0004: wal_decoding: Documentation for replication slots and changeset extraction
>
> The usage of pg_create_decoding_replication_slot does show the "(1 row)" line.

I mean "doesn't show" of course. :)

--
Thom

On February 7, 2014 9:58:14 PM CET, Thom Brown <thom(at)linux(dot)com> wrote:
>On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> 0004: wal_decoding: Documentation for replication slots and changeset
>extraction
>
>The usage of pg_create_decoding_replication_slot does show the "(1
>row)" line.
>
>The output of "SELECT * FROM pg_replication_slots;" is out-of-date.
>
>There appears to be a column named "slot_name" and "slottype". Could
>one of these have or not have the underscore for consistency?
>
>The example also shows output from pg_decoding_slot_get_changes after
>inserting 2 rows, but when I run the same example, there are no rows
>returned:
>
># BEGIN;
>BEGIN
>
>*# INSERT INTO data(data) VALUES('1');
>INSERT 0 1
>
>*# INSERT INTO data(data) VALUES('1');
>INSERT 0 1
>
>*# COMMIT;
>COMMIT
>
># SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
>'include-xids', '0');
> location | xid | data
>----------+-----+------
>(0 rows)
>
>
>I inserted a single row outside of a transaction, and got the expected
>output. Then I ran the above again, and got an output, but an
>unexpected one:
>
>SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
>'include-xids', '0');
> location | xid | data
>-----------+-----+-----------------------------------------------
> 0/16C8B90 | 769 | BEGIN
> 0/16C8D50 | 769 | table "data": INSERT: id[int4]:3 data[text]:1
> 0/16C8D50 | 769 | COMMIT
>(3 rows)
>
>And running the transaction with inserts again, there's no output from
>that same function command. I always get an output from isolated
>INSERT statements. I should point out that in my .psqlrc file I have
>"\set ON_ERROR_ROLLBACK". If I use psql -X, this symptom no longer
>occurs, so I think the automatic savepoints are interfering, and the
>effect appears to be inconsistent.

More complete answer later, but any chance you're using synchronous commit = off?

Thanks for looking,

Andres

--
Please excuse brevity and formatting - I am writing this on my mobile phone.

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

On 7 February 2014 21:04, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On February 7, 2014 9:58:14 PM CET, Thom Brown <thom(at)linux(dot)com> wrote:
>>On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> 0004: wal_decoding: Documentation for replication slots and changeset
>>extraction
>>
>>The usage of pg_create_decoding_replication_slot does show the "(1
>>row)" line.
>>
>>The output of "SELECT * FROM pg_replication_slots;" is out-of-date.
>>
>>There appears to be a column named "slot_name" and "slottype". Could
>>one of these have or not have the underscore for consistency?
>>
>>The example also shows output from pg_decoding_slot_get_changes after
>>inserting 2 rows, but when I run the same example, there are no rows
>>returned:
>>
>># BEGIN;
>>BEGIN
>>
>>*# INSERT INTO data(data) VALUES('1');
>>INSERT 0 1
>>
>>*# INSERT INTO data(data) VALUES('1');
>>INSERT 0 1
>>
>>*# COMMIT;
>>COMMIT
>>
>># SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
>>'include-xids', '0');
>> location | xid | data
>>----------+-----+------
>>(0 rows)
>>
>>
>>I inserted a single row outside of a transaction, and got the expected
>>output. Then I ran the above again, and got an output, but an
>>unexpected one:
>>
>>SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now',
>>'include-xids', '0');
>> location | xid | data
>>-----------+-----+-----------------------------------------------
>> 0/16C8B90 | 769 | BEGIN
>> 0/16C8D50 | 769 | table "data": INSERT: id[int4]:3 data[text]:1
>> 0/16C8D50 | 769 | COMMIT
>>(3 rows)
>>
>>And running the transaction with inserts again, there's no output from
>>that same function command. I always get an output from isolated
>>INSERT statements. I should point out that in my .psqlrc file I have
>>"\set ON_ERROR_ROLLBACK". If I use psql -X, this symptom no longer
>>occurs, so I think the automatic savepoints are interfering, and the
>>effect appears to be inconsistent.
>
> More complete answer later, but any chance you're using synchronous commit = off?

No:

# show synchronous_commit ;
synchronous_commit
--------------------
on
(1 row)

My custom config is:

wal_level = 'logical'
max_replication_slots = '1'
shared_buffers = 3900MB
temp_buffers = 16MB
work_mem = 16MB
maintenance_work_mem = 256MB
checkpoint_segments = 32
random_page_cost = 1.1
effective_cache_size = 12GB
logging_collector = on
log_line_prefix = '%t [%p]: [%l-1] user=%u,db=%d,client=%h '

--
Thom

On Fri, February 7, 2014 22:09, Thom Brown wrote:

>>The example also shows output from pg_decoding_slot_get_changes after
>>inserting 2 rows, but when I run the same example, there are no rows

FWIW, works for me:

testdb=# SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now', 'include-xids', '0');
location | xid | data
----------+-----+------
(0 rows)

testdb=# BEGIN; INSERT INTO data(data) VALUES('1'); INSERT INTO data(data) VALUES('1'); COMMIT;
testdb=# SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now', 'include-xids', '0');
location | xid | data
-----------+------+------------------------------------------------
0/2B81ED0 | 1973 | BEGIN
0/2B823A8 | 1973 | table "data": INSERT: id[int4]:14 data[text]:1
0/2B823A8 | 1973 | table "data": INSERT: id[int4]:15 data[text]:1
0/2B823A8 | 1973 | COMMIT
(4 rows)

testdb=# SELECT * FROM pg_decoding_slot_get_changes('regression_slot', 'now', 'include-xids', '0');
location | xid | data
----------+-----+------
(0 rows)

( output of "SELECT * FROM pg_replication_slots;" is, indeed, out-of-date.)

On 7 February 2014 21:28, Erik Rijkers <er(at)xs4all(dot)nl> wrote:
> On Fri, February 7, 2014 22:09, Thom Brown wrote:
>
>>>The example also shows output from pg_decoding_slot_get_changes after
>>>inserting 2 rows, but when I run the same example, there are no rows
>
> FWIW, works for me:

Can you confirm you're running it with ON_ERROR_ROLLBACK set?

--
Thom

On Fri, February 7, 2014 22:29, Thom Brown wrote:
> On 7 February 2014 21:28, Erik Rijkers <er(at)xs4all(dot)nl> wrote:
>> On Fri, February 7, 2014 22:09, Thom Brown wrote:
>>
>>>>The example also shows output from pg_decoding_slot_get_changes after
>>>>inserting 2 rows, but when I run the same example, there are no rows
>>
>> FWIW, works for me:
>
> Can you confirm you're running it with ON_ERROR_ROLLBACK set?
>

Ah, no, I missed that. You're right: with that, behaviour is the same here as you described.

On 2014-02-07 20:58:14 +0000, Thom Brown wrote:
> On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > 0004: wal_decoding: Documentation for replication slots and changeset extraction
>
> The usage of pg_create_decoding_replication_slot does show the "(1 row)" line.
>
> The output of "SELECT * FROM pg_replication_slots;" is out-of-date.

Thanks, refreshed.

> There appears to be a column named "slot_name" and "slottype". Could
> one of these have or not have the underscore for consistency?

That's luckily already fixed...

> The example also shows output from pg_decoding_slot_get_changes after
> inserting 2 rows, but when I run the same example, there are no rows
> returned:

> And running the transaction with inserts again, there's no output from
> that same function command. I always get an output from isolated
> INSERT statements. I should point out that in my .psqlrc file I have
> "\set ON_ERROR_ROLLBACK". If I use psql -X, this symptom no longer
> occurs, so I think the automatic savepoints are interfering, and the
> effect appears to be inconsistent.

Thanks, that's a bug indeed. I have experimentally fixed the bug, not
sure whether I like the fix yet, or not.

I've already fixed two issues caused by the rebase onto
858ec11858a914d4c380971985709b6d6b7dd6fc.

Is pushing to git sufficient for you, or shall I rebase and resend the
series?

Thanks!

Andres Freund

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

On 7 February 2014 23:43, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-07 20:58:14 +0000, Thom Brown wrote:
>> On 7 February 2014 19:35, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > 0004: wal_decoding: Documentation for replication slots and changeset extraction
>>
>> The usage of pg_create_decoding_replication_slot does show the "(1 row)" line.
>>
>> The output of "SELECT * FROM pg_replication_slots;" is out-of-date.
>
> Thanks, refreshed.
>
>> There appears to be a column named "slot_name" and "slottype". Could
>> one of these have or not have the underscore for consistency?
>
> That's luckily already fixed...
>
>> The example also shows output from pg_decoding_slot_get_changes after
>> inserting 2 rows, but when I run the same example, there are no rows
>> returned:
>
>> And running the transaction with inserts again, there's no output from
>> that same function command. I always get an output from isolated
>> INSERT statements. I should point out that in my .psqlrc file I have
>> "\set ON_ERROR_ROLLBACK". If I use psql -X, this symptom no longer
>> occurs, so I think the automatic savepoints are interfering, and the
>> effect appears to be inconsistent.
>
> Thanks, that's a bug indeed. I have experimentally fixed the bug, not
> sure whether I like the fix yet, or not.
>
> I've already fixed two issues caused by the rebase onto
> 858ec11858a914d4c380971985709b6d6b7dd6fc.
>
> Is pushing to git sufficient for you, or shall I rebase and resend the
> series?

Sure, push it to git, I'll add your remote repo and checkout that branch.

--
Thom

Hi,

Only got to this now, was a bit too tired and needed to catch up on some
real-world stuff...

On 2014-02-08 00:16:07 +0000, Thom Brown wrote:
> On 7 February 2014 23:43, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > Thanks, that's a bug indeed. I have experimentally fixed the bug, not
> > sure whether I like the fix yet, or not.
> >
> > I've already fixed two issues caused by the rebase onto
> > 858ec11858a914d4c380971985709b6d6b7dd6fc.
> >
> > Is pushing to git sufficient for you, or shall I rebase and resend the
> > series?
>
> Sure, push it to git, I'll add your remote repo and checkout that branch.

Ok, I roughly went with my initial plan to fix this and I've added (and
fixed) a regression for this.

Pushed this and some other improvements to
http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
branch xlog-decoding-rebasing-remapping

Thanks!

Andres Freund

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

On 8 February 2014 17:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi,
>
> Only got to this now, was a bit too tired and needed to catch up on some
> real-world stuff...
>
> On 2014-02-08 00:16:07 +0000, Thom Brown wrote:
>> On 7 February 2014 23:43, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> > Thanks, that's a bug indeed. I have experimentally fixed the bug, not
>> > sure whether I like the fix yet, or not.
>> >
>> > I've already fixed two issues caused by the rebase onto
>> > 858ec11858a914d4c380971985709b6d6b7dd6fc.
>> >
>> > Is pushing to git sufficient for you, or shall I rebase and resend the
>> > series?
>>
>> Sure, push it to git, I'll add your remote repo and checkout that branch.
>
> Ok, I roughly went with my initial plan to fix this and I've added (and
> fixed) a regression for this.
>
> Pushed this and some other improvements to
> http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
> branch xlog-decoding-rebasing-remapping

This appears to be working now. Thanks.

I'll continue to play around with the feature.

--
Thom

On 8 February 2014 19:35, Thom Brown <thom(at)linux(dot)com> wrote:
> On 8 February 2014 17:52, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>> Hi,
>>
>> Only got to this now, was a bit too tired and needed to catch up on some
>> real-world stuff...
>>
>> On 2014-02-08 00:16:07 +0000, Thom Brown wrote:
>>> On 7 February 2014 23:43, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
>>> > Thanks, that's a bug indeed. I have experimentally fixed the bug, not
>>> > sure whether I like the fix yet, or not.
>>> >
>>> > I've already fixed two issues caused by the rebase onto
>>> > 858ec11858a914d4c380971985709b6d6b7dd6fc.
>>> >
>>> > Is pushing to git sufficient for you, or shall I rebase and resend the
>>> > series?
>>>
>>> Sure, push it to git, I'll add your remote repo and checkout that branch.
>>
>> Ok, I roughly went with my initial plan to fix this and I've added (and
>> fixed) a regression for this.
>>
>> Pushed this and some other improvements to
>> http://git.postgresql.org/gitweb/?p=users/andresfreund/postgres.git;a=summary
>> branch xlog-decoding-rebasing-remapping
>
> This appears to be working now. Thanks.
>
> I'll continue to play around with the feature.

Next issue. Firstly, an out-of-date example:

doc/src/sgml/changesetextraction.sgml

pg_recvlogical --slot test --init -d testdb

There's no option --init. I think this is supposed to be --create.

But also:

$ pg_recvlogical --slot test --create -d testdb
pg_recvlogical: could not send replication command
"CREATE_REPLICATION_SLOT "test" LOGICAL "test_decoding"": extraneous
data in "T" message

But this seems to have created it anyway:

# SELECT * FROM pg_replication_slots;
slot_name | plugin | slot_type | datoid | database | active |
xmin | catalog_xmin | restart_lsn
-----------+---------------+-----------+--------+----------+--------+------+--------------+-------------
test | test_decoding | logical | 16396 | testdb | t |
| | 0/16E2030
(1 row)

If I drop it and run the same command, the same message is emitted.

--
Thom

On 2014-02-08 21:07:03 +0000, Thom Brown wrote:
> > I'll continue to play around with the feature.
>
> Next issue. Firstly, an out-of-date example:
>
> doc/src/sgml/changesetextraction.sgml
>
> pg_recvlogical --slot test --init -d testdb
>
> There's no option --init. I think this is supposed to be --create.

Fixed. It used to be --init, but that has changed. Thanks.

> $ pg_recvlogical --slot test --create -d testdb
> pg_recvlogical: could not send replication command
> "CREATE_REPLICATION_SLOT "test" LOGICAL "test_decoding"": extraneous
> data in "T" message

Gah. Another merge issue. Fixed. We really need to have infrastructure
for testing binaries...

Thanks,

Andres Freund

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

On 8 February 2014 21:25, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-08 21:07:03 +0000, Thom Brown wrote:
>> > I'll continue to play around with the feature.
>>
>> Next issue. Firstly, an out-of-date example:
>>
>> doc/src/sgml/changesetextraction.sgml
>>
>> pg_recvlogical --slot test --init -d testdb
>>
>> There's no option --init. I think this is supposed to be --create.
>
> Fixed. It used to be --init, but that has changed. Thanks.
>
>> $ pg_recvlogical --slot test --create -d testdb
>> pg_recvlogical: could not send replication command
>> "CREATE_REPLICATION_SLOT "test" LOGICAL "test_decoding"": extraneous
>> data in "T" message
>
> Gah. Another merge issue. Fixed. We really need to have infrastructure
> for testing binaries...

Thanks, no issue with that now.

Got a question about ranges and arrays usage with timestamps... why
are quotes added to these?

timestamptz (no quotes with input or output):
table "a": INSERT: moo[timestamptz]:2014-02-08 22:09:33+00

tstzrange (no quotes with input, but quotes with output):
table "b": INSERT: moo[tstzrange]:["2014-02-08
13:45:22+00","2014-02-08 14:45:42+00")

timestamptz[] (no quotes with input, but quotes with output):
table "c": INSERT: moo[_timestamptz]:{"2010-01-01
13:45:22+00","2010-01-03 14:45:42+00"}

tstzrange[] (one set of quotes with input, two sets of quotes with
output, one set of which are escaped):
table "d": INSERT: moo[_tstzrange]:{"(\"2014-02-08
13:45:22+00\",\"2014-02-08 13:45:42+00\"]","[\"2014-02-07
10:12:19+00\",\"2014-02-07 13:51:16+00\"]"}

--
Thom

Hi Thom,

On 2014-02-08 22:26:59 +0000, Thom Brown wrote:
> Got a question about ranges and arrays usage with timestamps... why
> are quotes added to these?
>
> timestamptz (no quotes with input or output):
> table "a": INSERT: moo[timestamptz]:2014-02-08 22:09:33+00
>
> tstzrange (no quotes with input, but quotes with output):
> table "b": INSERT: moo[tstzrange]:["2014-02-08
> 13:45:22+00","2014-02-08 14:45:42+00")
>
> timestamptz[] (no quotes with input, but quotes with output):
> table "c": INSERT: moo[_timestamptz]:{"2010-01-01
> 13:45:22+00","2010-01-03 14:45:42+00"}
>
> tstzrange[] (one set of quotes with input, two sets of quotes with
> output, one set of which are escaped):
> table "d": INSERT: moo[_tstzrange]:{"(\"2014-02-08
> 13:45:22+00\",\"2014-02-08 13:45:42+00\"]","[\"2014-02-07
> 10:12:19+00\",\"2014-02-07 13:51:16+00\"]"}

The test_decoding output plugin just uses the default text output
functions for all types, other plugins could do differently. I.e. in all
these cases a SELECT, COPY, pg_dump will also include those quotes.
E.g.
postgres=# SELECT ARRAY[tstzrange(NOW(), NOW() + interval '1 day')];
will format it's output similarly:
{"[\"2014-02-08 23:44:19.82007+01\",\"2014-02-09 23:44:19.82007+01\")"}
(1 row)

Does that answer make sense?

Greetings,

Andres Freund

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

On 8 February 2014 22:47, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> Hi Thom,
>
> On 2014-02-08 22:26:59 +0000, Thom Brown wrote:
>> Got a question about ranges and arrays usage with timestamps... why
>> are quotes added to these?
>>
>> timestamptz (no quotes with input or output):
>> table "a": INSERT: moo[timestamptz]:2014-02-08 22:09:33+00
>>
>> tstzrange (no quotes with input, but quotes with output):
>> table "b": INSERT: moo[tstzrange]:["2014-02-08
>> 13:45:22+00","2014-02-08 14:45:42+00")
>>
>> timestamptz[] (no quotes with input, but quotes with output):
>> table "c": INSERT: moo[_timestamptz]:{"2010-01-01
>> 13:45:22+00","2010-01-03 14:45:42+00"}
>>
>> tstzrange[] (one set of quotes with input, two sets of quotes with
>> output, one set of which are escaped):
>> table "d": INSERT: moo[_tstzrange]:{"(\"2014-02-08
>> 13:45:22+00\",\"2014-02-08 13:45:42+00\"]","[\"2014-02-07
>> 10:12:19+00\",\"2014-02-07 13:51:16+00\"]"}
>
> The test_decoding output plugin just uses the default text output
> functions for all types, other plugins could do differently. I.e. in all
> these cases a SELECT, COPY, pg_dump will also include those quotes.
> E.g.
> postgres=# SELECT ARRAY[tstzrange(NOW(), NOW() + interval '1 day')];
> will format it's output similarly:
> {"[\"2014-02-08 23:44:19.82007+01\",\"2014-02-09 23:44:19.82007+01\")"}
> (1 row)
>
> Does that answer make sense?

Ah, okay. Thanks.

Another question: in order for logical decoding/replication to be
useful, presumably one would need a primary key on every table? It's
just I haven't seen this mentioned on the changeset extraction page in
the docs.

--
Thom

On 2014-02-08 22:58:35 +0000, Thom Brown wrote:
> Another question: in order for logical decoding/replication to be
> useful, presumably one would need a primary key on every table? It's
> just I haven't seen this mentioned on the changeset extraction page in
> the docs.

Hm, that's a good point. 07cacba983ef79be4a84fcd0e0ca3b5fcb85dd65 added
configurability for that, but there at least should be a link to
http://www.postgresql.org/docs/devel/static/sql-altertable.html with
some additional words.

Greetings,

Andres Freund

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

On 8 February 2014 23:08, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-08 22:58:35 +0000, Thom Brown wrote:
>> Another question: in order for logical decoding/replication to be
>> useful, presumably one would need a primary key on every table? It's
>> just I haven't seen this mentioned on the changeset extraction page in
>> the docs.
>
> Hm, that's a good point. 07cacba983ef79be4a84fcd0e0ca3b5fcb85dd65 added
> configurability for that, but there at least should be a link to
> http://www.postgresql.org/docs/devel/static/sql-altertable.html with
> some additional words.

# CREATE TABLE test (id serial primary key, val int);
CREATE TABLE

# INSERT INTO test (val) SELECT generate_series(1,3);
INSERT 0 3

# ALTER TABLE test ADD COLUMN a decimal DEFAULT 2.22;
ALTER TABLE

# ALTER TABLE test ADD COLUMN b json DEFAULT '{"a":[1,2,3],"b":[4,5,6]}';
ALTER TABLE

The output generated by those last 2 statements is:

BEGIN 891
table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
COMMIT 891
BEGIN 892
table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
b[json]:{"a":[1,2,3],"b":[4,5,6]}
table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
b[json]:{"a":[1,2,3],"b":[4,5,6]}
table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
b[json]:{"a":[1,2,3],"b":[4,5,6]}
COMMIT 892

This is showing inserts into the temp table as part of the operation.
Is that sufficient?

--
Thom

On 2014-02-09 00:49:31 +0000, Thom Brown wrote:
> # ALTER TABLE test ADD COLUMN a decimal DEFAULT 2.22;
> ALTER TABLE
>
> # ALTER TABLE test ADD COLUMN b json DEFAULT '{"a":[1,2,3],"b":[4,5,6]}';
> ALTER TABLE
>
> The output generated by those last 2 statements is:
>
> BEGIN 891
> table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
> table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
> table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
> COMMIT 891
> BEGIN 892
> table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
> b[json]:{"a":[1,2,3],"b":[4,5,6]}
> table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
> b[json]:{"a":[1,2,3],"b":[4,5,6]}
> table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
> b[json]:{"a":[1,2,3],"b":[4,5,6]}
> COMMIT 892
>
> This is showing inserts into the temp table as part of the operation.
> Is that sufficient?

I think it's a good thing for now. We don't have support for DDL
replication so it's not yet that interesting, but having the new values
allows to safely handle things like DEFAULTs that produce
nondeterministic data.
What do you think?

Greetings,

Andres Freund

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

On 9 February 2014 01:06, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> On 2014-02-09 00:49:31 +0000, Thom Brown wrote:
>> # ALTER TABLE test ADD COLUMN a decimal DEFAULT 2.22;
>> ALTER TABLE
>>
>> # ALTER TABLE test ADD COLUMN b json DEFAULT '{"a":[1,2,3],"b":[4,5,6]}';
>> ALTER TABLE
>>
>> The output generated by those last 2 statements is:
>>
>> BEGIN 891
>> table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
>> table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
>> table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
>> COMMIT 891
>> BEGIN 892
>> table "pg_temp_16552": INSERT: id[int4]:1 val[int4]:1 a[numeric]:2.22
>> b[json]:{"a":[1,2,3],"b":[4,5,6]}
>> table "pg_temp_16552": INSERT: id[int4]:2 val[int4]:2 a[numeric]:2.22
>> b[json]:{"a":[1,2,3],"b":[4,5,6]}
>> table "pg_temp_16552": INSERT: id[int4]:3 val[int4]:3 a[numeric]:2.22
>> b[json]:{"a":[1,2,3],"b":[4,5,6]}
>> COMMIT 892
>>
>> This is showing inserts into the temp table as part of the operation.
>> Is that sufficient?
>
> I think it's a good thing for now. We don't have support for DDL
> replication so it's not yet that interesting, but having the new values
> allows to safely handle things like DEFAULTs that produce
> nondeterministic data.
> What do you think?

Okay, I'm just checking. If it's expected behaviour to you, it's good
enough for me.

--
Thom

On Fri, Feb 7, 2014 at 2:35 PM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> attached you can find the next version of the patchset.

As usual, I'm going to be reviewing patch 1. The definition of "patch
1" has changed quite a few times over the past year, but that's
usually the one I'm reviewing.

+ * contents of records in here xexcept turning them into a more usable

Typo.

+ /*
+ * XXX: There doesn't seem to be a usecase for decoding
+ * HEAP_NEWPAGE's. Its only used in various
indexam's and CLUSTER,
+ * neither of which should be relevant for the logical
+ * changestream.
+ */

There's a level of uncertainty here that doesn't seem consistent with
calling this a finished patch. It's also not a complete list of
places where log_newpage() is called, but frankly I don't think that
should be the aim of this comment. The only relevant question is
whether we ever use XLOG_HEAP_NEWPAGE to log heap changes that are
relevant to logical replication. I think we don't.

+ /* FIXME: skip if wrong db? */

It's time to fish or cut bait.

+ /*
+ * XXX: As a future feature, we could replay
the transaction and
+ * prepare it as well, allowing for 2PC via
logical decoding.
+ */

Let's try to avoid using XXX (or FIXME) for things that really mean TODO.

I think this comment deserves to be expanded a bit, too. Maybe
something like: "Right now, logical decoding ignores PREPARE
TRANSACTION and simply decodes the subsequent COMMIT TRANSACTION or
ROLLBACK TRANSACTION just as it would a regular COMMIT or ROLLBACK.
In the future, we might want to change this. Decoding PREPARE might
enable future code to prepare each locally prepared transaction on the
remote side before doing a COMMIT TRANSACTION locally, allowing for
logical synchronous replication."

+ /*
+ * If the record wasn't part of a transaction,
it will not have
+ * caused invalidations and thus isn't
important when building
+ * snapshots. If it was part of a transaction,
that transaction
+ * just performed DDL because those are the
only codepaths using
+ * inplace updates.
+ */

Under what circumstances do we issue in-place updates not associated
with a transaction? And under what circumstances do we issue in-place
updates that ARE associated with a transaction?

+ * XXX: At some point we might want to execute the transaction's

The XXX again seems needless; the comment is fine as it stands.

+ /*
+ * Abort all transactions that we keep
track of that are older
+ * than ->oldestRunningXid. This is
the most convenient spot

I think writing ->membername is poor commenting style. Just leave out
the arrow, or write "the WAL record's oldestRunningXid."

+/*
+ * Get the data from the various forms of commit records and pass it
+ * on to snapbuild.c and reorderbuffer.c
+ */

This is a lousy comment. I suggest something like: "Currently, each
transaction is decoded only once it commit, so the arrival of a commit
record means that we can now decode the changes made by this toplevel
transaction and all of its committed subtransactions, unless we have
to skip it because the replication system isn't fully initialized yet.
Whether decoding the transaction or not, we must take note of any
invalidations it issues, as those will affect the snapshot used for
decoding of *other* transactions."

+/*
+ * Get the data from the various forms of abort records and pass it on to
+ * snapbuild.c and reorderbuffer.c
+ */

Suggest: "When a transaction abort is detected, we throw away any data
we've stashed away for possible future decoding of that transaction.
Knowledge of the abort may also help us establish our initial snapshot
when logical decoding is first initiated."

+/*
+ * Set the xmin required for decoding snapshots for the specific decoding
+ * slot.
+ */
+void
+IncreaseLogicalXminForSlot(XLogRecPtr lsn, TransactionId xmin)

I'm thinking this and everything that follows, up through
LogicalDecodingCountDBSlots, probably should be moved to slot.c.

+ /* XXX: Add the current LSN? */

+1.

+ /* shorter lines... */
+ slot = MyReplicationSlot;

If you're going to do this, which seems like it's probably a good
idea, do it at the top of the function and use it all the way through
instead of doing it in the middle.

+ if (MyReplicationSlot == NULL)
+ elog(ERROR, "need a current slot");
+
+ if (is_init && start_lsn != InvalidXLogRecPtr)
+ elog(ERROR, "Cannot INIT_LOGICAL_REPLICATION at a
specified LSN");
+
+ if (is_init && plugin == NULL)
+ elog(ERROR, "Cannot INIT_LOGICAL_REPLICATION without a
specified plugin");

One of these error messages is not like the others.

+ context = AllocSetContextCreate(CurrentMemoryContext,
+
"Changeset Extraction Context",
+
ALLOCSET_DEFAULT_MINSIZE,
+
ALLOCSET_DEFAULT_INITSIZE,
+
ALLOCSET_DEFAULT_MAXSIZE);

I have my doubts about whether it's wise to make this the child of
CurrentMemoryContext. Certainly, if we do that, then expectations
around what that context is need to be documented. Short-lived
contexts are presumably unsuitable.

+ * Lets start with enough information if we can, so
log a standby
+ * snapshot and start decoding at exactl that position.

Let's. Exactly.

+ * the xlog records didn't result in anyting
relevant for

anything.

+ elog(LOG, "cannot stream from %X/%X, minimum
is %X/%X, forwarding",

This isn't very clear, and I don't think it follows style guidelines either.

+ /* register output plugin name with slot */
+ strncpy(NameStr(MyReplicationSlot->data.plugin), plugin,
+ NAMEDATALEN);
+ NameStr(MyReplicationSlot->data.plugin)[NAMEDATALEN - 1] = '\0';

Hmm. Shouldn't this be delegated to something in slot.c? Why don't
we need the lock? Why don't we need to fsync() the change?

+ /*
+ * Acquire the current global xmin value and directly
set the logical
+ * xmin before releasing the lock if necessary. We do
this so wal
+ * decoding is guaranteed to have all catalog rows
produced by xacts
+ * with an xid > walsnd->xmin available.
+ *
+ * We can't let ReplicationSlotsComputeRequiredXmin() lock the
+ * procarray as that acquires ProcArrayLock separately
which would
+ * open a short window for the global xmin to advance
above our xmin.
+ */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ slot->effective_catalog_xmin =
GetOldestSafeDecodingTransactionId();
+ slot->data.catalog_xmin = slot->effective_catalog_xmin;
+
+ ReplicationSlotsComputeRequiredXmin(true);
+
+ LWLockRelease(ProcArrayLock);

I don't understand this.

+ (errmsg("changeset extraction started,
extracting changes after %X/%X, reading from %X/%X",

Needs some style work. What does "extracting changes after %X/%X"
really mean? Also, how about including the slot name in there?

+ * Performoutput plugin write into tuplestore.

Space.

+ /*
+ * XXX: maybe we ought to assert ctx->out is in database encoding when
+ * we're writing textual output.
+ */

Good idea.

+ /*
+ * FIXME: we're going to have to do something more
intelligent about
+ * timelines on standby's. Use readTimeLineHistory() and
+ * tliOfPointInHistory() to get the proper LSN?
+ */

So what's the plan for that?

+ /*
+ * XXX: It'd be way nicer to be able to use the
walsender waiting logic
+ * here, but that's not available in all environments.
+ */

I don't understand this.

pg_create_decoding_replication_slot() should go in slotfuncs.c.

+/* number of changes kept in memory, per transaction */
+const Size max_memtries = 4096;
+
+/* Size of the slab caches used for frequently allocated objects */
+const Size max_cached_changes = 4096 * 2;
+const Size max_cached_tuplebufs = 4096 * 2; /* ~8MB */
+const Size max_cached_transactions = 512;

Hmm. Is max_memtries the number of "tries" you keep in "mem"? Or is
"tries" an inadvertent abbreviation for "entries" or something? Also,
there's no real discussion here of the logic behind these values, or
the performance or memory impact of changing them.

+ * Free an ReorderBufferTXN. Deallocation might be delayed for efficiency
+ * purposes.

Wow, so we have a bespoke dllist for caching these objects, and that's
actually material to performance? What is the allocation/deallocation
rate here?

+/*
+ * FIXME: better comment and/or name
+ */

If not now, then when?

+ ReorderBufferChange *next_change =
+ dlist_container(ReorderBufferChange, node, next);

Formatting.

+ * ->subxip contains all txids that belong to our transaction which we

snap->subxip

+ * XXX: ->nsubxcnt can be out of date when subtransactions abort, count
+ * manually.

Why is this an XXX?

+ if (GetTopTransactionIdIfAny() != InvalidTransactionId)
+ elog(ERROR, "cannot replay using sub,
already allocated xid %u",
+ GetTopTransactionIdIfAny());

Message style. "cannot replay using top", too. Also, what makes this
elog() material? If decoding can be performed from a user session
this seems likely to be (blech!) user-facing.

+ /* XXX: we could skip
snapshots in non toplevel txns */

TODO. Or fix it.

+ /*
+ * don't do a ReorderBufferCleanupTXN here, with the
vague idea of
+ * allowing to retry decoding.
+ */

It's a bit late in the cycle for such vagueness.

+ * Rejigger change->newtuple to point to in-memory toast tuples instead to
+ * on-disk toast tuples that may not longer exist (think DROP TABLE or VACUUM).
+ *
+ * We cannot replace unchanged toast tuples though, so those will still point
+ * to on-disk toast data.

Why is that OK?

+ * location indicated by 'lsn'. Returns true if successfull, false otherwise.

Extra "l".

My eyes are glazing over, so I'm stopping here for now.

Generally, I think there's a lot of good stuff in here, but I think
you need to make a serious pass through here and try to get rid of all
the things marked XXX and FIXME, either by changing them to say TODO
(or nothing), or by deciding that they're OK and removing the comment,
or by fixing them. It's fine to have some XXX comments on points
where you're hoping for reviewer feedback, but the time to have notes
in there for yourself is long past. Also, you need to either go
through and make a studious attempt to fix all the typographical
errors and message style issues, or you need to find someone else who
can help you with that. Preferably not me, because I'd rather focus
on things of somewhat greater significance.

Generally, I find decode.c to be relatively straightforward, and it's
even got a nice long header comment explaining what it does.
logical.c, on the other hand, has a one-line header comment. As I
mentioned above, I think a lot of the stuff in this file properly
belongs elsewhere, so maybe that's not so bad. But it could probably
use at least a little more work.

To the extent that it's possible, the documentation should be
incorporated into the patches that introduce the corresponding
facilities rather than being a separate patch all of its own.

The meat of the patch seems to be reorderbuffer.c. That file needs
more and better explanations of what is being done and why it is being
done. For example:

+/*
+ * Abort a transaction that possibly has previous changes. Needs to be done
+ * independently for toplevel and subtransactions.
+ */
+void
+ReorderBufferAbort(ReorderBuffer *rb, TransactionId xid, XLogRecPtr lsn)

It's already clear from the name of the function and how it's used
elsewhere that it gets called when a transaction aborts. Mentioning
that the function gets invoked for both toplevel and subtransactions
is, surely, worthwhile. But the function has no comments explaining
what it's doing in response to that abort, or why it's doing those
things, or why it's doing those things in that order rather than some
other order. Here is an example of a better comment:

+/*
+ * Setup the base snapshot of a transaction. That is the snapshot that is used
+ * to decode all changes until either this transaction modifies the catalog or
+ * another catalog modifying transaction commits.
+ */

Now, the grammar there might need a tad of work, but there's a lot of
useful information in that comment. It would be even better if there
were a comment explaining, either here or in the caller, how we decide
*when* to call this function.

Both reorderbuffer.c and snapbuild.c contain a significant number of
functions that are quite short. I can't decide whether this is
excellent attention to abstraction boundaries or a sign that the
abstraction boundary is too permeable in the first place.

This email is a bit down in the trenches; I will try to write another
with some higher-level considerations. But I think there is plenty of
stuff here for you to get started fixing.

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

Hi!

On 2014-02-11 11:22:24 -0500, Robert Haas wrote:
> + * contents of records in here xexcept turning them into a more usable
>
> Typo.
>
> + /*
> + * XXX: There doesn't seem to be a usecase for decoding
> + * HEAP_NEWPAGE's. Its only used in various
> indexam's and CLUSTER,
> + * neither of which should be relevant for the logical
> + * changestream.
> + */
>
> There's a level of uncertainty here that doesn't seem consistent with
> calling this a finished patch. It's also not a complete list of
> places where log_newpage() is called, but frankly I don't think that
> should be the aim of this comment. The only relevant question is
> whether we ever use XLOG_HEAP_NEWPAGE to log heap changes that are
> relevant to logical replication. I think we don't.

You're right, we currently don't. I guess we should add a comment to
log_newpage()/buffer to make sure that's not violated in future code,
that seems like the only place that's somewhat likely to be read?

> Decoding PREPARE might
> enable future code to prepare each locally prepared transaction on the
> remote side before doing a COMMIT TRANSACTION locally, allowing for
> logical synchronous replication."

We do support logical synchronous replication over the walsender
interface, what it would allow us to do would be some form of 2pc...

I'll adapt your version.

> + /*
> + * If the record wasn't part of a transaction,
> it will not have
> + * caused invalidations and thus isn't
> important when building
> + * snapshots. If it was part of a transaction,
> that transaction
> + * just performed DDL because those are the
> only codepaths using
> + * inplace updates.
> + */
>
> Under what circumstances do we issue in-place updates not associated
> with a transaction? And under what circumstances do we issue in-place
> updates that ARE associated with a transaction?

vacuum updates relfrozenxid outside a transaction, e.g. create/drop
index, analyze inside one.

> +/*
> + * Get the data from the various forms of commit records and pass it
> + * on to snapbuild.c and reorderbuffer.c
> + */
>
> This is a lousy comment. I suggest something like: "Currently, each
> transaction is decoded only once it commit, so the arrival of a commit
> record means that we can now decode the changes made by this toplevel
> transaction and all of its committed subtransactions, unless we have
> to skip it because the replication system isn't fully initialized yet.
> Whether decoding the transaction or not, we must take note of any
> invalidations it issues, as those will affect the snapshot used for
> decoding of *other* transactions."
>
> +/*
> + * Get the data from the various forms of abort records and pass it on to
> + * snapbuild.c and reorderbuffer.c
> + */
>
> Suggest: "When a transaction abort is detected, we throw away any data
> we've stashed away for possible future decoding of that transaction.
> Knowledge of the abort may also help us establish our initial snapshot
> when logical decoding is first initiated."

Hm, those should go into reorderbuffer.c instead, the interesting part
of DecodeCommit/DecodeAbort is just to centralize the handling of the
various forms of commit/abort records. decode.c really shouldn't need to
be changed much when we start to optionally support streaming out changes
immediately.

> + /* register output plugin name with slot */
> + strncpy(NameStr(MyReplicationSlot->data.plugin), plugin,
> + NAMEDATALEN);
> + NameStr(MyReplicationSlot->data.plugin)[NAMEDATALEN - 1] = '\0';
>
> Hmm. Shouldn't this be delegated to something in slot.c? Why don't
> we need the lock?

I wasn't sure, we can place it there, but it doesn't really need to know
about these details either. Lockingwise I don't see it needing more,
nobody but the slot that has it acquired is interested in it.

> Why don't we need to fsync() the change?

I've since pushed a patch that does the fsyncing. Not doing so was part
of a rebase screwup.

> + /*
> + * FIXME: we're going to have to do something more
> intelligent about
> + * timelines on standby's. Use readTimeLineHistory() and
> + * tliOfPointInHistory() to get the proper LSN?
> + */
>
> So what's the plan for that?

Prohibit decoding on the standby for now. Not sure how to deal with the
relevant code, leave it there, #ifdef it out, remove it?

> + /*
> + * XXX: It'd be way nicer to be able to use the
> walsender waiting logic
> + * here, but that's not available in all environments.
> + */
>
> I don't understand this.

The walsender get's notified when flushing WAL, which allows the
walsender specific read_page callback to wait on the walsender
latch. For normal backends that's not available, so we have to do a
check/sleep/repeat logic.

> pg_create_decoding_replication_slot() should go in slotfuncs.c.

I wasn't sure about where to place it.

> + * Free an ReorderBufferTXN. Deallocation might be delayed for efficiency
> + * purposes.
>
> Wow, so we have a bespoke dllist for caching these objects, and that's
> actually material to performance? What is the allocation/deallocation
> rate here?

It's absolutely massively relevant for performance, I was really
surprised. reorderbuffer.c was the original reason for developing
ilist.h...
Before doing this, memory allocation was by far the top profile,
afterwards it has left the top ten.

I've tried my memory manager rewrite on it, but it didn't fix things
sufficiently. I don't think there's anything as good as a per-type slab
allocation (which this essentially boils down to, even if it has a
higher memory overhead) for frequently allocated types.

> + * XXX: ->nsubxcnt can be out of date when subtransactions abort, count
> + * manually.
>
> Why is this an XXX?

For me XXX really is "watch out", that's the only reason.

> + /*
> + * don't do a ReorderBufferCleanupTXN here, with the
> vague idea of
> + * allowing to retry decoding.
> + */
>
> It's a bit late in the cycle for such vagueness.

Well, I sure think people (including me) will continue to work on
this. There's not much downside to not doing a cleanup here, it will be
cleaned up later.

> + * Rejigger change->newtuple to point to in-memory toast tuples instead to
> + * on-disk toast tuples that may not longer exist (think DROP TABLE or VACUUM).
> + *
> + * We cannot replace unchanged toast tuples though, so those will still point
> + * to on-disk toast data.
>
> Why is that OK?

Because we currently only allow accessing changed toast tuples. We'd
discussed that a long time back and it seemed people agree that that's
fair enough initially. In fact, more people were interested in that
behaviour than in doing it differently.
Alternatively we can vacuum toast tables less agressively or WAL log
more.

> This email is a bit down in the trenches; I will try to write another
> with some higher-level considerations. But I think there is plenty of
> stuff here for you to get started fixing.

Yes, working on it. I'll push the smaller increments to git, ok?

Working on all the issues now, thanks!

Andres Freund

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

On 2014-02-11 11:22:24 -0500, Robert Haas wrote:
> + context = AllocSetContextCreate(CurrentMemoryContext,
> +
> "Changeset Extraction Context",
> +
> ALLOCSET_DEFAULT_MINSIZE,
> +
> ALLOCSET_DEFAULT_INITSIZE,
> +
> ALLOCSET_DEFAULT_MAXSIZE);
>
> I have my doubts about whether it's wise to make this the child of
> CurrentMemoryContext. Certainly, if we do that, then expectations
> around what that context is need to be documented. Short-lived
> contexts are presumably unsuitable.

Well, it depends on the type of usage. In the walsender, yes, it needs
to be a longliving context. Not so much in the SQL case, inside the SRF
we spill all the data into a tuplestore after which we are done. I don't
see which context would be more suitable as a default parent; it used to
be TopMemoryContext but that requires pointless cleanup routines to
handle errors...

So I think documenting the requirement is the best way?

I'm working on the other comments, pushing individual changes to
git. Will send a new version onlist once I'm through.

Greetings,

Andres Freund

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

Hi,

On 2014-02-11 11:22:24 -0500, Robert Haas wrote:
> [loads of comments]

I tried to address all the points you mentioned.

Except:
* I ended up only moving some functions from logical.c to slot.c, not sure
if it's the right split now.
* I couldn't merge the docs entirely to the commits, that'd be a
horrible mess right now, since the the new sgml file refers to all the
tools and output methods. I think that'd make changing things too hard.

News:
* loads of comment, error reporting, documentation improvements
* output plugins now have to specify whether they output data textually
or in binary so the text SQL interface functions can refuse if it's binary.
* changeset extraction while in recovery is disallowed for now
* some more tests were added
Bugs fixed:
* pg_recvlogical could accidently close stdout if -f - was specified and
the connection died.
* the WAL reservation wasn't working when initially creation a slot,
only a bit later.
* typo in pg_replication_slots lead to catalog_xmin not being displayed.
* 8de3e410faa06ab20ec1aa6d0abb0a2c040261ba required some minor changes

Thanks to Christian Kruse for helping me!

Greetings,

Andres Freund

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

On Sun, 2014-01-19 at 15:31 +0100, Stefan Kaltenbrunner wrote:
> > /* followings are for client encoding only */
> > PG_SJIS, /* Shift JIS
> > (Winindows-932) */
>
> while you have that file open: s/Winindows-932/Windows-932 maybe?

done

On Thu, February 13, 2014 17:12, Andres Freund wrote:
> Hi,
>
> On 2014-02-11 11:22:24 -0500, Robert Haas wrote:
>> [loads of comments]
>
> I tried to address all the points you mentioned.
>

>0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k
>0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k
>0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k
>0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k
>0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k

These don't apply...

Hi,

On 2014-02-14 09:23:45 +0100, Erik Rijkers wrote:
> >0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k
> >0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k
> >0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k
> >0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k
> >0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k
>
> These don't apply...

Works here, could you give a bit more details about the problem you have
applying them? Note that they are compressed, so need to be gunzipped first...

Greetings,

Andres Freund

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

On Fri, February 14, 2014 10:13, Andres Freund wrote:
> Hi,
>
> On 2014-02-14 09:23:45 +0100, Erik Rijkers wrote:
>> >0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k
>> >0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k
>> >0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k
>> >0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k
>> >0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k
>>
>> These don't apply...
>
> Works here, could you give a bit more details about the problem you have
> applying them? Note that they are compressed, so need to be gunzipped first...
>

yeah, unzipping -- I thought of that :) (no offense taken :))

I just gave it into my standard patch-applying-compiling shell script which does something like, as always :

patch --dry-run -b -l -F 25 -p 1 <
/home/aardvark/download/pgpatches/0094/lcsg/20140213/0001-wal_decoding-Introduce-logical-changeset-extraction.patch
>patch.1.dry-run.1.out

(it loops to try out several -p values, none acceptable)

etc

On 2014-02-14 10:42:46 +0100, Erik Rijkers wrote:
> On Fri, February 14, 2014 10:13, Andres Freund wrote:
> > Hi,
> >
> > On 2014-02-14 09:23:45 +0100, Erik Rijkers wrote:
> >> >0001-wal_decoding-Introduce-logical-changeset-extraction.patch.gz 159 k
> >> >0002-wal_decoding-logical-changeset-extraction-walsender-.patch.gz 16 k
> >> >0003-wal_decoding-pg_recvlogical-Introduce-pg_receivexlog.patch.gz 15 k
> >> >0004-wal_decoding-Documentation-for-replication-slots-and.patch.gz 14 k
> >> >0005-wal_decoding-Temporarily-add-logical-decoding-regres.patch.gz 1.8 k
> >>
> >> These don't apply...
> >
> > Works here, could you give a bit more details about the problem you have
> > applying them? Note that they are compressed, so need to be gunzipped first...
> >
>
> yeah, unzipping -- I thought of that :) (no offense taken :))

Good ;). I thought it was better to ask...

I hadn't yet fetched new changes since tonight, that's why it worked for
me. It's breaking because of 801c2dc72cb3c68a7c430bb244675b7a68fd541a.

Greetings,

Andres Freund

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

On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> [ new patches ]

0001 already needs minor

+ * copied stuff from tuptoaster.c. Perhaps there should be toast_internal.h?

Yes, please. If you can submit a separate patch creating this file
and relocating this stuff there, I will commit it.

+ /*
+ * XXX: It's impolite to ignore our argument and keep decoding until the
+ * current position.
+ */

Eh, what?

+ * We misuse the original meaning of SnapshotData's xip and
subxip fields
+ * to make the more fitting for our needs.
[...]
+ * XXX: Do we want extra fields instead of misusing existing
ones instead?

If we're going to do this, then it surely needs to be documented in
snapshot.h. On the second question, you're not the first hacker to
want to abuse the meanings of the existing fields; SnapshotDirty
already does it. It's tempting to think we need a more principled
approach to this, like what we've done with Node i.e. typedef enum ...
SnapshotType; and then a separate struct definition for each kind, all
beginning with SnapshotType type.

+ /*
+ * XXX: Timeline handling/naming. Do we need to include the timeline in
+ * snapshot's name? Outside of very obscure, user triggered, cases every
+ * LSN should correspond to exactly one timeline?
+ */

I can't really comment intelligently on this, so you need to figure it
out. My off-the-cuff thought is that erring on the side of including
it couldn't hurt anything.

+ * XXX: use hex format for the LSN as well?

Yes, please.

+ /* prepared abort contain a normal
commit abort... */

contains.

+ /*
+ * Abort all transactions that we keep
track of that are older
+ * than the record's oldestRunningXid.
This is the most
+ * convenient spot for doing so since,
in contrast to shutdown
+ * or end-of-recovery checkpoints, we
have sufficient
+ * knowledge to deal with prepared
transactions here.
+ */

I have no real quibble with this, but I think the comment could be
clarified slightly to state *what* knowledge we have here that we
wouldn't have there.

+ /* only crash recovery/replication needs to care */

I believe I know what you're getting at here, but the next guy might
not. I suggest: "Although these records only exist to serve the needs
of logical decoding, all the work happens as part of crash or archive
recovery, so we don't need to do anything here."

+ * Treat HOT update as normal updates, there
is no useful

s/, t/. T/

+ * There are cases in which inplace updates
are used without xids
+ * assigned (like VACUUM), there are others
(like CREATE INDEX
+ * CONCURRENTLY) where an xid is present. If
an xid was assigned

In-place updates can be used either by XID-bearing transactions (e.g.
in CREATE INDEX CONCURRENTLY) or by XID-less transactions (e.g.
VACUUM). In the former case, ...

+ * redundant because the commit will do that
as well, but one
+ * we'll support decoding in-progress
relations, this will be

s/one/once/
s/we'll/we/

+ /* we don't care about row level locks for now */
+ case XLOG_HEAP_LOCK:
+ break;

The position of the comment isn't consistent with the comments for the
other WAL record type in this section; that is, it's above rather than
below the case.

+ * transaction's contents as the various caches need to always be

I think you should use "since" or "because" rather than "as" here, and
maybe put a comma before it.

+ * the transaction's invalidations. This currently won't be needed if
+ * we're just skipping over the transaction, since that currently only
+ * happens when starting decoding, after we invalidated all caches, but
+ * transactions in other databases might have touched shared relations.

I'm having trouble understanding what this means, especially the part
after the "but".

+ * Read a HeapTuple as WAL logged by heap_insert, heap_update and
+ * heap_delete, but not by heap_multi_insert into a tuplebuf.

"but not by heap_multi_insert" needs punctuation both before and
after. You can just add a comma after, or change it into a
parenthetical phrase.

As the above comments probably make clear, I'm pretty much happy with decode.c.

+ /* TODO: We got to change that someday soon.. */

Two periods. Maybe "We need to change this some day soon." - and then
follow that with a paragraph explaining what roughly what would need
to be done.

+ /* shorter lines... */
+ slot = MyReplicationSlot;
+
+ /* first some sanity checks that are unlikely to be violated */
+ if (MyReplicationSlot == NULL)
+ elog(ERROR, "cannot perform logical decoding without a
acquired slot");

Can test slot.

+ /* make sure the passed slot is suitable, these are user
facing errors */

Make sure the passed slot is suitable. These are user-facing errors.

+ if (IsTransactionState() &&
+ GetTopTransactionIdIfAny() != InvalidTransactionId)
+ ereport(ERROR,
+ (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
+ errmsg("cannot perform changeset
extraction in transaction that has performed writes")));

This is sort of an awkward restriction, as it makes it hard to compose
this feature with others. What underlies the restriction, can we get
rid of it, and if not can we include a comment here explaining why it
exists?

+ * the xlog records didn't result in anyting
relevant for

anything.

+ /* register output plugin name with slot */
+ strncpy(NameStr(slot->data.plugin), plugin,
+ NAMEDATALEN);
+ NameStr(slot->data.plugin)[NAMEDATALEN - 1] = '\0';

If it's safe to do this without a lock, I don't know why.

More broadly, I wonder why this is_init stuff is in here at all.
Maybe the stuff that happens in the is_init case should be done in the
caller, or another helper function.

+ /* prevent WAL removal as fast as possible */
+ ReplicationSlotsComputeRequiredLSN();

If there's a race here, can't we rejigger the order of operations to
eliminate it? Or is that just too hard and not worth it?

+begin_txn_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
+ state.callback_name = "pg_decode_begin_txn";
+ ctx->callbacks.begin_cb(ctx, txn);

I feel that there's a certain lack of naming consistency between these
things. Can we improve that? (and similarly for parallel cases)

+pg_create_decoding_replication_slot(PG_FUNCTION_ARGS)

I thought we were going to have physical and logical slots, not
physical and decoding slots.

+ /* make sure we don't end up with an unreleased slot */
+ PG_TRY();
+ {
...
+ PG_CATCH();
+ {
+ ReplicationSlotRelease();
+ ReplicationSlotDrop(NameStr(*name));
+ PG_RE_THROW();
+ }
+ PG_END_TRY();

I don't think this is a very good idea. The problem with doing things
during error recovery that can themselves fail is that you'll lose the
original error, which is not cool, and maybe even blow out the error
stack. Many people have confuse PG_TRY()/PG_CATCH() with an
exception-handling system, but it's not. One way to fix this is to
put some of the initialization logic in ReplicationSlotCreate() just
prior to calling CreateSlotOnDisk(). If the work that needs to be
done is too complex or protracted to be done there, then I think that
it should be pulled out of the act of creating the replication slot
and made to happen as part of first use, or as a separate operation
like PrepareForLogicalDecoding.

+ * When the client has confirmed flushes >= candidate_xmin_after we can

candidate_xmin_after is not otherwise referenced; incomplete identifier rename?

Nothing in patch 1 sets PROC_IN_LOGICAL_DECODING. Is that right?

+ProcArraySetReplicationSlotXmin(TransactionId xmin, TransactionId catalog_xmin,
+ bool
already_locked)

Maybe Assert(!already_locked || LWLockHeldByMe(ProcArrayLock))

+void
+ProcArrayGetReplicationSlotXmin(TransactionId *xmin,
+
TransactionId *catalog_xmin)
+{
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);

If we need the lock in exclusive mode here, there should be a comment
explaining why. And regardless, there should probably be some sort of
header comment.

+ /*
+ * Acquire spinlock so other backends are guaranteed
to see this in
+ * time - we cannot generally acquire the lwlock here
since we might
+ * be still holding it in an error path.
+ */
+ SpinLockAcquire(&MyReplicationSlot->mutex);
+ MyReplicationSlot->active = false;
+ SpinLockRelease(&MyReplicationSlot->mutex);
+
+ /* might not have been set when we've been a plain slot */
+ LWLockAcquire(ProcArrayLock, LW_EXCLUSIVE);
+ MyPgXact->vacuumFlags &= ~PROC_IN_LOGICAL_DECODING;
+ LWLockRelease(ProcArrayLock);

OK, a couple of things. First, the comment for the first chunk says
we can't acquire the LWLock here, and then the second part acquires an
LWLock. Second, if we're about to dump our PGPROC altogether, why do
we need to update vacuumFlags first? Third, this isn't actually
called anywhere.

+ * At some point in the future it probaly makes sense to have a more elaborate
+ * resource management here, but it's not entirely clear how that would look
+ * like.

s/how/what/

ReorderBufferGetTXN() should get a comment about the performance
impact of this. There's a tiny bit there in ReorderBufferReturnTXN()
but it should be better called out. Should these call the valgrind
macros to make the memory inaccessible while it's being held in cache?

My eyes are starting to glaze over, so more later.

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

On 2014-02-15 17:29:04 -0500, Robert Haas wrote:
> On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > [ new patches ]
>
> 0001 already needs minor

Hm?

If there are conflicts, I'll push/send a rebased tomorrow or monday.

> + * the transaction's invalidations. This currently won't be needed if
> + * we're just skipping over the transaction, since that currently only
> + * happens when starting decoding, after we invalidated all caches, but
> + * transactions in other databases might have touched shared relations.
>
> I'm having trouble understanding what this means, especially the part
> after the "but".

Let me rephrase, maybe that will help:

This currently won't be needed if we're just skipping over the
transaction because currenlty we only do so during startup, to get to
the first transaction the client needs. As we have reset the catalog
caches before starting to read WAL and we haven't yet touched any
catalogs there can't be anything to invalidate.

But if we're "forgetting" this commit because it's it happened in
another database, the invalidations might be important, because they
could be for shared catalogs and we might have loaded data into the
relevant syscaches.

Better?

> + if (IsTransactionState() &&
> + GetTopTransactionIdIfAny() != InvalidTransactionId)
> + ereport(ERROR,
> + (errcode(ERRCODE_ACTIVE_SQL_TRANSACTION),
> + errmsg("cannot perform changeset
> extraction in transaction that has performed writes")));
>
> This is sort of an awkward restriction, as it makes it hard to compose
> this feature with others. What underlies the restriction, can we get
> rid of it, and if not can we include a comment here explaining why it
> exists?

Well, you can write the result into a table if you're halfway
careful... :)

I think it should be fairly easy to relax the restriction to creating a
slot, but not getting data from it. Do you think that would that be
sufficient?

> + /* register output plugin name with slot */
> + strncpy(NameStr(slot->data.plugin), plugin,
> + NAMEDATALEN);
> + NameStr(slot->data.plugin)[NAMEDATALEN - 1] = '\0';
>
> If it's safe to do this without a lock, I don't know why.

Well, the worst that could happen is that somebody else doing a SELECT *
FROM pg_replication_slot gets a incomplete plugin name... But we
certainly can hold the spinlock during it if you think that's better.

> More broadly, I wonder why this is_init stuff is in here at all.
> Maybe the stuff that happens in the is_init case should be done in the
> caller, or another helper function.

The reason I moved it in there is that after the walsender patch there
are two callers and the stuff is sufficiently complex that I having it
twice lead to bugs.
The reason it's currenlty the same function is that sufficiently much of
the code would have to be shared that I found it it ugly to split. I'll
have a look whether I can figure something out.

> + /* prevent WAL removal as fast as possible */
> + ReplicationSlotsComputeRequiredLSN();
>
> If there's a race here, can't we rejigger the order of operations to
> eliminate it? Or is that just too hard and not worth it?

Yes, there's a small race which at the very least should be properly
documented.

Hm. Yes, I think we can plug the hole. If the race condition occurs we'd
take slightly longer to startup, which isn't bad. Will fix.

> +begin_txn_wrapper(ReorderBuffer *cache, ReorderBufferTXN *txn)
> + state.callback_name = "pg_decode_begin_txn";
> + ctx->callbacks.begin_cb(ctx, txn);
>
> I feel that there's a certain lack of naming consistency between these
> things. Can we improve that? (and similarly for parallel cases)
>
> +pg_create_decoding_replication_slot(PG_FUNCTION_ARGS)
>
> I thought we were going to have physical and logical slots, not
> physical and decoding slots.

Ok.

> + /* make sure we don't end up with an unreleased slot */
> + PG_TRY();
> + {
> ...
> + PG_CATCH();
> + {
> + ReplicationSlotRelease();
> + ReplicationSlotDrop(NameStr(*name));
> + PG_RE_THROW();
> + }
> + PG_END_TRY();
>
> I don't think this is a very good idea. The problem with doing things
> during error recovery that can themselves fail is that you'll lose the
> original error, which is not cool, and maybe even blow out the error
> stack. Many people have confuse PG_TRY()/PG_CATCH() with an
> exception-handling system, but it's not. One way to fix this is to
> put some of the initialization logic in ReplicationSlotCreate() just
> prior to calling CreateSlotOnDisk(). If the work that needs to be
> done is too complex or protracted to be done there, then I think that
> it should be pulled out of the act of creating the replication slot
> and made to happen as part of first use, or as a separate operation
> like PrepareForLogicalDecoding.

I think what should be done here is adding a drop_on_release flag. As
soon as everything important is done, it gets unset.

With some small changes that'd be highly useful for pg_basebackup as
well, because it could simply create a slot to prevent removal of
important WAL. Hm, maybe name it release_on_error, and call it from the
error locations.
You previously (IM?) argued that that'd be problematic because of locks,
but in all the error handling situations we'll already have released
lwlocks. And we rely on being able to tacke lwlocks there,
c.f. TerminateBufferIO et al.

> + * When the client has confirmed flushes >= candidate_xmin_after we can
>
> candidate_xmin_after is not otherwise referenced; incomplete identifier rename?
>
> Nothing in patch 1 sets PROC_IN_LOGICAL_DECODING. Is that right?

CreateDecodingContext() does.

> ReorderBufferGetTXN() should get a comment about the performance
> impact of this. There's a tiny bit there in ReorderBufferReturnTXN()
> but it should be better called out. Should these call the valgrind
> macros to make the memory inaccessible while it's being held in cache?

Hm, I think it does call the valgrind stuff?
VALGRIND_MAKE_MEM_UNDEFINED(txn, sizeof(ReorderBufferTXN));
VALGRIND_MAKE_MEM_DEFINED(&txn->node, sizeof(txn->node));

> My eyes are starting to glaze over, so more later.

Thanks! Will address asap.

Greetings,

Andres Freund

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

On 2014-02-15 17:29:04 -0500, Robert Haas wrote:
> On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> > [ new patches ]
>
> 0001 already needs minor
>
> + * copied stuff from tuptoaster.c. Perhaps there should be toast_internal.h?
>
> Yes, please. If you can submit a separate patch creating this file
> and relocating this stuff there, I will commit it.

I started to work on that, but I am not sure we actually need it
anymore. tuptoaster.h isn't included in that many places, so perhaps we
should just add it there?

Greetings,

Andres Freund

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

On Fri, Feb 14, 2014 at 4:55 AM, Andres Freund <andres(at)2ndquadrant(dot)com> wrote:
> [ patches ]

Having now had a little bit of opportunity to reflect on the State Of
This Patch, I'd like to step back from the minutia upon which I've
been commenting in my previous emails and articulate three high-level
concerns about this patch. In so doing, I would like to specifically
request that other folks on this mailing list comment on the extent to
which they do or do not believe these concerns to be valid. I believe
I've mentioned all of these concerns at least to some degree
previously, but they've been mixed in with other things, so I want to
take this opportunity to call them out more clearly.

1. How safe is it to try to do decoding inside of a regular backend?
What we're doing here is entering a special mode where we forbid the
use of regular snapshots in favor of requiring the use of "decoding
snapshots", and forbid access to non-catalog relations. We then run
through the decoding process; and then exit back into regular mode.
On entering and on exiting this special mode, we
InvalidateSystemCaches(). I don't see a big problem with having
special backends (e.g. walsender) use this special mode, but I'm less
convinced that it's wise to try to set things up so that we can switch
back and forth between decoding mode and regular mode in a single
backend. I worry that won't end up working out very cleanly, and I
think the prohibition against using this special mode in an
XID-bearing transaction is merely a small downpayment on future pain
in this area. That having been said, I can't pretend at this point
either to understand the genesis of this particular restriction or
what other problems are likely to crop up in trying to allow this
mode-switching. So it's possible that I'm overblowing it, but it's
makin' me nervous.

2. I think the snapshot-export code is fundamentally misdesigned. As
I said before, the idea that we're going to export one single snapshot
at one particular point in time strikes me as extremely short-sighted.
For example, consider one-to-many replication where clients may join
or depart the replication group at any time. Whenever somebody joins,
we just want a <snapshot, LSN> pair such that they can apply all
changes after the LSN except for XIDs that would have been visible to
the snapshot. And in fact, we don't even need any special machinery
for that; the client can just make a connection and *take a snapshot*
once decoding is initialized enough. This code is going to great
pains to be able to export a snapshot at the precise point when all
transactions that were running in the first xl_running_xacts record
seen after the start of decoding have ended, but there's nothing
magical about that point, except that it's the first point at which a
freshly-taken snapshot is guaranteed to be good enough to establish an
initial state for any table in the database.

But do you really want to keep that snapshot around long enough to
copy the entire database? I bet you don't: if the database is big,
holding back xmin for long enough to copy the whole thing isn't likely
to be fun. You might well want to copy one table at a time, with
progressively newer snapshots, and apply to each table only those
transactions that weren't part of the initial snapshot for that table.
Many other patterns are possible. What you've got baked in here
right now is suitable only for the simplest imaginable case, and yet
we're paying a substantial price in implementation complexity for it.
Frankly, this code is *ugly*; the fact that SnapBuildExportSnapshot()
needs to start a transaction so that it can push out a snapshot. I
think that's a pretty awful abuse of the transaction machinery, and
the whole point of it, AFAICS, is to eliminate flexibility that we'd
have with simpler approaches.

3. As this feature is proposed, the only plugin we'll ship with 9.4 is
a test_decoding plugin which, as its own documentation says, "doesn't
do anything especially useful." What exactly do we gain by forcing
users who want to make use of these new capabilities to write C code?
You previously stated that it wasn't possible (or there wasn't time)
to write something generic, but how hard is it, really? Sure, people
who are hard-core should have the option to write C code, and I'm
happy that they do. But that shouldn't, IMHO anyway, be a requirement
to use that feature, and I'm having trouble understanding why we're
making it one. The test_decoding plugin doesn't seem tremendously
much simpler than something that someone could actually use, so why
not make that the goal?

Thanks,

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

Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> Having now had a little bit of opportunity to reflect on the State Of
> This Patch, I'd like to step back from the minutia upon which I've
> been commenting in my previous emails and articulate three high-level
> concerns about this patch. In so doing, I would like to specifically
> request that other folks on this mailing list comment on the extent to
> which they do or do not believe these concerns to be valid.
> ...

> 1. How safe is it to try to do decoding inside of a regular backend?
> What we're doing here is entering a special mode where we forbid the
> use of regular snapshots in favor of requiring the use of "decoding
> snapshots", and forbid access to non-catalog relations. We then run
> through the decoding process; and then exit back into regular mode.
> On entering and on exiting this special mode, we
> InvalidateSystemCaches().

How often is such a mode switch expected to happen? I would expect
frequent use of InvalidateSystemCaches() to be pretty much disastrous
for performance, even absent any of the possible bugs you're worried
about. It would likely be better to design things so that a decoder
backend does only that.

> 2. I think the snapshot-export code is fundamentally misdesigned.

Your concerns here sound reasonable, but I can't say I've got any
special insight into it.

> 3. As this feature is proposed, the only plugin we'll ship with 9.4 is
> a test_decoding plugin which, as its own documentation says, "doesn't
> do anything especially useful." What exactly do we gain by forcing
> users who want to make use of these new capabilities to write C code?

TBH, if that's all we're going to ship, I'm going to vote against
committing this patch to 9.4 at all. Let it wait till 9.5 when we
might be able to build something useful on it. To point out just
one obvious problem, how much confidence can we have in the APIs
being right if there are no usable clients? Even if they're right,
what benefit do we get from freezing them one release before anything
useful is going to happen?

The most recent precedent I can think of is the FDW APIs, which I'd
be the first to admit are still in flux. But we didn't ship anything
there without non-toy contrib modules to exercise it. If we had,
we'd certainly have regretted it, because in the creation of those
contrib modules we found flaws in the initial design.

regards, tom lane

On Mon, Feb 17, 2014 at 9:10 PM, Tom Lane <tgl(at)sss(dot)pgh(dot)pa(dot)us> wrote:
>> 3. As this feature is proposed, the only plugin we'll ship with 9.4 is
>> a test_decoding plugin which, as its own documentation says, "doesn't
>> do anything especially useful." What exactly do we gain by forcing
>> users who want to make use of these new capabilities to write C code?
>
> TBH, if that's all we're going to ship, I'm going to vote against
> committing this patch to 9.4 at all. Let it wait till 9.5 when we
> might be able to build something useful on it. To point out just
> one obvious problem, how much confidence can we have in the APIs
> being right if there are no usable clients? Even if they're right,
> what benefit do we get from freezing them one release before anything
> useful is going to happen?

I actually have a lot of confidence that the APIs are almost entirely
right, except maybe for the snapshot-related stuff and possibly one or
two other minor details. And I have every confidence that 2ndQuadrant
is going to put out decoding modules that do useful stuff. I also
assume Slony is going to ship one at some point. EnterpriseDB's xDB
replication server will need one, so someone at EDB will have to go
write that. And if Bucardo or Londiste want to use this
infrastructure, they'll need their own, too. What I don't understand
is why it's cool to make each of those replication solutions bring its
own to the table. I mean if they want to, so that they can generate
exactly the format they want with no extra overhead, sure, cool. What
I don't understand is why we're not taking the test_decoding module,
polishing it up a little to produce some nice, easily
machine-parseable output, calling it basic_decoding, and shipping
that. Then people who want something else can build it, but people
who are happy with something basic will already have it.

What I actually suspect is going to happen if we ship this as-is is
that people are going to start building logical replication solutions
on top of the test_decoding module even though it explicitly says that
it's just test code. This is *really* cool technology and people are
*hungry* for it. But writing C is hard, so if there's not a polished
plugin available, I bet people are going to try to use the
not-polished one. I think we try to get out ahead of that.

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

On Mon, Feb 17, 2014 at 6:35 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> What I actually suspect is going to happen if we ship this as-is is
> that people are going to start building logical replication solutions
> on top of the test_decoding module even though it explicitly says that
> it's just test code. This is *really* cool technology and people are
> *hungry* for it. But writing C is hard, so if there's not a polished
> plugin available, I bet people are going to try to use the
> not-polished one. I think we try to get out ahead of that.

Tom made a comparison with FDWs, so I'll make another. The Multicorn
module made FDW authorship much more accessible by wrapping it in a
Python interface, I believe with some success. I don't want to stand
in the way of building a fully-featured test_decoding module, but I
think that those that would misuse test_decoding as it currently
stands can be redirected to a third-party wrapper. As you say, it's
pretty cool stuff, so it seems likely that someone will build one for
us.

--
Peter Geoghegan

Hi Robert,

On 2014-02-17 20:31:34 -0500, Robert Haas wrote:
> 1. How safe is it to try to do decoding inside of a regular backend?
> What we're doing here is entering a special mode where we forbid the
> use of regular snapshots in favor of requiring the use of "decoding
> snapshots", and forbid access to non-catalog relations. We then run
> through the decoding process; and then exit back into regular mode.
> On entering and on exiting this special mode, we
> InvalidateSystemCaches(). I don't see a big problem with having
> special backends (e.g. walsender) use this special mode, but I'm less
> convinced that it's wise to try to set things up so that we can switch
> back and forth between decoding mode and regular mode in a single
> backend.

The main reason the SQL interface exists is that it's awfully hard to
use isolationtester, pg_regress et al when the output isn't also visible
via SQL. We tried hacking things in other ways, but that's what it came
down to. If you recall, previously the SQL changes interface was only in
a test_logical_decoding extension, because I wasn't sure it's all that
interesting for real usecases.
It's sure nice for testing things though.

> I worry that won't end up working out very cleanly, and I
> think the prohibition against using this special mode in an
> XID-bearing transaction is merely a small downpayment on future pain
> in this area.

That restriction is in principle only needed when creating the slot, not
when getting changes. The only problem is that some piece of code
doesn't know about it.

The reason it exists are twofold: One is that when looking for an
initial snapshot, we wait for concurrent transactions to end. If we'd
wait for the transaction itself we'd be in trouble, it could never
happen. The second reason is that the code do a XactLockTableWait() to
"visualize" it's waiting, so isolatester knows it should background the
command. It's not good to wait on itself.
But neither is actually needed when not creating the slot, the code just
needs to be told about that.

> That having been said, I can't pretend at this point
> either to understand the genesis of this particular restriction or
> what other problems are likely to crop up in trying to allow this
> mode-switching. So it's possible that I'm overblowing it, but it's
> makin' me nervous.

I am not terribly concerned, but I can understand where you are coming
from. I think for replication solutions this isn't going to be needed
but it's way much more handy for testing and such.

> 2. I think the snapshot-export code is fundamentally misdesigned. As
> I said before, the idea that we're going to export one single snapshot
> at one particular point in time strikes me as extremely short-sighted.

I don't think so. It's precisely what you need to implement a simple
replication solution. Yes, there are usecases that could benefit from
more possibilities, but that's always the case.

> For example, consider one-to-many replication where clients may join
> or depart the replication group at any time. Whenever somebody joins,
> we just want a <snapshot, LSN> pair such that they can apply all
> changes after the LSN except for XIDs that would have been visible to
> the snapshot.

And? They need to create individual replication slots, which each will
get a snapshot.

> And in fact, we don't even need any special machinery
> for that; the client can just make a connection and *take a snapshot*
> once decoding is initialized enough.

No, they can't. Two reasons: For one the commit order between snapshots
and WAL isn't necessarily the same. For another, clients now need logic
to detect whether a transaction's contents has already been applied or
has not been applied yet, that's nontrivial.

> This code is going to great
> pains to be able to export a snapshot at the precise point when all
> transactions that were running in the first xl_running_xacts record
> seen after the start of decoding have ended, but there's nothing
> magical about that point, except that it's the first point at which a
> freshly-taken snapshot is guaranteed to be good enough to establish an
> initial state for any table in the database.

I still maintain that there's something magic about that moment. It's
when all *future* (from the POV of the snapshot) changes will be
streamed, and all *past* changes are included in the exported snapshot.

> But do you really want to keep that snapshot around long enough to
> copy the entire database? I bet you don't: if the database is big,
> holding back xmin for long enough to copy the whole thing isn't likely
> to be fun.

Well, that's how pg_dump works, it's not this patch's problem to fix
that.

> You might well want to copy one table at a time, with
> progressively newer snapshots, and apply to each table only those
> transactions that weren't part of the initial snapshot for that table.
> Many other patterns are possible. What you've got baked in here
> right now is suitable only for the simplest imaginable case, and yet
> we're paying a substantial price in implementation complexity for it.

Which implementation complexity are you talking about? The relevant code
is maybe 50-60 lines?

> Frankly, this code is *ugly*; the fact that SnapBuildExportSnapshot()
> needs to start a transaction so that it can push out a snapshot. I
> think that's a pretty awful abuse of the transaction machinery, and
> the whole point of it, AFAICS, is to eliminate flexibility that we'd
> have with simpler approaches.

It's not my idea that the snapshot importing requires that
restriction. We could possibly lift it and replace it by another check,
but I don't really see the problem.

> 3. As this feature is proposed, the only plugin we'll ship with 9.4 is
> a test_decoding plugin which, as its own documentation says, "doesn't
> do anything especially useful." What exactly do we gain by forcing
> users who want to make use of these new capabilities to write C code?

It gains us to have a output plugin in which we can easily demonstrate
features so they can be tested in the regression tests. Which I find to
be rather important.
Just like e.g. the test_shm_mq stuff doesn't do anything really useful.

> You previously stated that it wasn't possible (or there wasn't time)
> to write something generic, but how hard is it, really? Sure, people
> who are hard-core should have the option to write C code, and I'm
> happy that they do. But that shouldn't, IMHO anyway, be a requirement
> to use that feature, and I'm having trouble understanding why we're
> making it one.

I think the commmunity will step up and provide further plugins. In
fact, there's already been a json plugin on the mailinglist.

> The test_decoding plugin doesn't seem tremendously
> much simpler than something that someone could actually use, so why
> not make that the goal?

For one, it being a designated toy plugin allows us to easily change it,
to showcase/test new features. For another, I still don't agree that
it's easy to agree to an output format. I think we should include some
that matured into 9.5.

Thanks,

Andres Freund

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

On 2014-02-17 21:10:26 -0500, Tom Lane wrote:
> Robert Haas <robertmhaas(at)gmail(dot)com> writes:
> > 1. How safe is it to try to do decoding inside of a regular backend?
> > What we're doing here is entering a special mode where we forbid the
> > use of regular snapshots in favor of requiring the use of "decoding
> > snapshots", and forbid access to non-catalog relations. We then run
> > through the decoding process; and then exit back into regular mode.
> > On entering and on exiting this special mode, we
> > InvalidateSystemCaches().
>
> How often is such a mode switch expected to happen? I would expect
> frequent use of InvalidateSystemCaches() to be pretty much disastrous
> for performance, even absent any of the possible bugs you're worried
> about. It would likely be better to design things so that a decoder
> backend does only that.

Very infrequently. When it's starting to decode, and when it's
ending. When used via walsender, that should only happen at connection
start/end which surely shouldn't be frequent.
It's more frequent when using the SQL interface, but since that's not a
streaming interface on a busy server there still would be a couple of
megabytes of transactions to decode for one reset.

> > 3. As this feature is proposed, the only plugin we'll ship with 9.4 is
> > a test_decoding plugin which, as its own documentation says, "doesn't
> > do anything especially useful." What exactly do we gain by forcing
> > users who want to make use of these new capabilities to write C code?
>
> TBH, if that's all we're going to ship, I'm going to vote against
> committing this patch to 9.4 at all. Let it wait till 9.5 when we
> might be able to build something useful on it.

There *are* useful things around already. We didn't include postgres_fdw
in the same release as the fdw code either? I don't see why this should
be held to a different standard.

> To point out just
> one obvious problem, how much confidence can we have in the APIs
> being right if there are no usable clients?

Because there *are* clients. They just don't sound likely to either be
suitable for core code (to specialized) or have already been submitted
(the json plugin).

There's a whole replication suite built ontop of this, to a good degree
to just test it. So I am fairly confident that the most important parts
are covered. There sure is additional features I want, but that's not
surprising.

> The most recent precedent I can think of is the FDW APIs, which I'd
> be the first to admit are still in flux. But we didn't ship anything
> there without non-toy contrib modules to exercise it. If we had,
> we'd certainly have regretted it, because in the creation of those
> contrib modules we found flaws in the initial design.

Which non-toy fdw was there? file_fdw was in 9.1, but that's a toy. And
*8.4* had CREATE FOREIGN DATA WRAPPER, without it doing anything...

Greetings,

Andres Freund

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

On 2014-02-17 18:49:59 -0800, Peter Geoghegan wrote:
> On Mon, Feb 17, 2014 at 6:35 PM, Robert Haas <robertmhaas(at)gmail(dot)com> wrote:
> > What I actually suspect is going to happen if we ship this as-is is
> > that people are going to start building logical replication solutions
> > on top of the test_decoding module even though it explicitly says that
> > it's just test code. This is *really* cool technology and people are
> > *hungry* for it. But writing C is hard, so if there's not a polished
> > plugin available, I bet people are going to try to use the
> > not-polished one. I think we try to get out ahead of that.
>
> Tom made a comparison with FDWs, so I'll make another. The Multicorn
> module made FDW authorship much more accessible by wrapping it in a
> Python interface, I believe with some success. I don't want to stand
> in the way of building a fully-featured test_decoding module, but I
> think that those that would misuse test_decoding as it currently
> stands can be redirected to a third-party wrapper. As you say, it's
> pretty cool stuff, so it seems likely that someone will build one for
> us.

Absolutely. I *sure* hope somebody is going to build such an
abstraction. I am not entirely sure how it'd look like, but ...

Greetings,

Andres Freund

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

On 2014-02-17 21:35:23 -0500, Robert Haas wrote:
> What
> I don't understand is why we're not taking the test_decoding module,
> polishing it up a little to produce some nice, easily
> machine-parseable output, calling it basic_decoding, and shipping
> that. Then people who want something else can build it, but people
> who are happy with something basic will already have it.

Because every project is going to need their own plugin
*anyway*. Londiste, slony sure are going to ignore changes to relations
they don't need. Querying their own metadata. They will want
compatibility to the earlier formats as far as possible. Sometime not
too far away they will want to optionally support binary output because
it's so much faster.
There's just not much chance that either of these will be able to agree
on a format short term.

So, possibly we could agree to something that consumers *outside* of
replication could use.

> What I actually suspect is going to happen if we ship this as-is is
> that people are going to start building logical replication solutions
> on top of the test_decoding module even though it explicitly says that
> it's just test code. This is *really* cool technology and people are
> *hungry* for it. But writing C is hard, so if there's not a polished
> plugin available, I bet people are going to try to use the
> not-polished one. I think we try to get out ahead of that.

I really hope there will be nicer ones by the time 9.4 is
released. Euler did send in a json plugin
http://archives.postgresql.org/message-id/52A5BFAE.1040209%2540timbira.com.br
, but there hasn't too much feedback yet. It's hard to start discussing
something that needs a couple of patches to pg before you can develop
your own patch...

Greetings,

Andres Freund

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