On Wed, Aug 12, 2015 at 8:20 AM, Andres Freund <andres@anarazel.de> wrote:
> On 2015-08-11 15:59:59 -0700, Gurjeet Singh wrote:
>> In your version, I don't see a comment that refers to the fact that it
>> works on the currently active (global variable) slot.
>
> Hm?
>
> /*
> * Reserve WAL for the currently active slot.
> *
> * Compute and set restart_lsn in a manner that's appropriate for the type of
> * the slot and concurrency safe.
> */
>> > I moved
>> > values[0] = NameGetDatum(&MyReplicationSlot->data.name);
>> > nulls[0] = false;
>> > to outside the conditional block, there seems no reason to have it in
>> > there?
>> >
>>
>> The assignment to values[0] is being done twice. We can do away with the
>> one in the else part of the if condition.
>
> Ugh, that's a mistake.
>
>> [1]: I altered you to it in a personal email, but probably it fell through
>> the cracks.
>
> I usually just check the lists for newer patch versions, sorry...
Commit 6fcd8851, which is the result of this thread, is not touching
the replication protocol at all. This looks like an oversight to me:
we should be a maximum consistent between the SQL interface and the
replication protocol if possible, and it looks useful to me to be able
to set restart_lsn when creating the slot as well when using a
replication connection. Most of the work has visibly been done with
the refactoring that created ReplicationSlotReserveWal, hence how
would we expose this option in CREATE_REPLICATION_SLOT?
For now we can do that:
CREATE_REPLICATION_SLOT IDENT K_PHYSICAL
We could append a keyword like RESERVE on this query. Or go through
more fancy things like (slot_options) where slot_options is a list of
option items, reserve = on/off.
Thoughts?
--
Michael