On Mon, Aug 10, 2015 at 7:12 AM, Andres Freund <and...@anarazel.de> wrote:
> On 2015-07-07 09:42:54 -0700, Gurjeet Singh wrote:
> > /*
> > + * Grab and save an LSN value to prevent WAL recycling past that point.
> > + */
> > +void
> > +ReplicationSlotRegisterRestartLSN()
> > +{
>
> Didn't like that description and function name very much. What does
> 'grabbing' mean here? Should probably mention that it works on the
> currently active slot and modifies it.
>
In your version, I don't see a comment that refers to the fact that it
works on the currently active (global variable) slot.
>
> It's now ReplicationSlotReserveWal()
>
Okay.
>
> > + ReplicationSlot *slot = MyReplicationSlot;
> > +
> > + Assert(slot != NULL);
> > + Assert(slot->data.restart_lsn == InvalidXLogRecPtr);
> > +
> > + /*
> > + * The replication slot mechanism is used to prevent removal of
> required
> > + * WAL. As there is no interlock between this and checkpoints
> required, WAL
> > + * segment could be removed before
> ReplicationSlotsComputeRequiredLSN() has
> > + * been called to prevent that. In the very unlikely case that
> this happens
> > + * we'll just retry.
> > + */
>
> You removed some punctuation in that sentence converting a sentence in
> bad english into one without the original meaning ;). See the attached
> for a new version.
>
Your version looks better.
>
> > +/*
> > * Flush all replication slots to disk.
> > *
> > * This needn't actually be part of a checkpoint, but it's a convenient
> > @@ -876,7 +942,7 @@ StartupReplicationSlots(void)
> > }
> >
> > /* ----
> > - * Manipulation of ondisk state of replication slots
> > + * Manipulation of on-disk state of replication slots
> > *
> > * NB: none of the routines below should take any notice whether a slot
> is the
> > * current one or not, that's all handled a layer above.
> > diff --git a/src/backend/replication/slotfuncs.c
> b/src/backend/replication/slotfuncs.c
> > index 9a2793f..01b376a 100644
> > --- a/src/backend/replication/slotfuncs.c
> > +++ b/src/backend/replication/slotfuncs.c
> > @@ -40,6 +40,7 @@ Datum
> > pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
> > {
> > Name name = PG_GETARG_NAME(0);
> > + bool immediately_reserve = PG_GETARG_BOOL(1);
> > Datum values[2];
> > bool nulls[2];
> > TupleDesc tupdesc;
> > @@ -58,10 +59,28 @@ pg_create_physical_replication_slot(PG_FUNCTION_ARGS)
> > /* acquire replication slot, this will check for conflicting names
> */
> > ReplicationSlotCreate(NameStr(*name), false, RS_PERSISTENT);
> >
> > - values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> > + if (immediately_reserve)
> > + {
> > + /* Allocate restart-LSN, if the user asked for it */
> > + ReplicationSlotRegisterRestartLSN();
> > +
> > + /* Write this slot to disk */
> > + ReplicationSlotMarkDirty();
> > + ReplicationSlotSave();
> >
> > - nulls[0] = false;
> > - nulls[1] = true;
> > + values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> > + values[1] =
> LSNGetDatum(MyReplicationSlot->data.restart_lsn);
> > +
> > + nulls[0] = false;
> > + nulls[1] = false;
> > + }
> > + else
> > + {
> > + values[0] = NameGetDatum(&MyReplicationSlot->data.name);
> > +
> > + nulls[0] = false;
> > + nulls[1] = true;
> > + }
>
> 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.
Also, there was a typo in my patch [1] and it seems to have made it into
the commit: <acronym<LSN</>. Tom seems to have just fixed it in
commit 750fc78b.
Best regards,
[1]: I altered you to it in a personal email, but probably it fell through
the cracks.
--
Gurjeet Singh http://gurjeet.singh.im/
