Re: PATCH: Add 'pid' column to pg_replication_slots

On 21 April 2015 at 15:19, Andres Freund <andres(at)anarazel(dot)de> wrote:

> On 2015-04-07 18:41:59 +0800, Craig Ringer wrote:
> > @@ -331,8 +331,8 @@ ReplicationSlotAcquire(const char *name)
> > volatile ReplicationSlot *vslot = s;
> >
> > SpinLockAcquire(&s->mutex);
> > - active = vslot->active;
> > - vslot->active = true;
> > + active = vslot->active_pid != 0;
> > + vslot->active_pid = MyProcPid;
> > SpinLockRelease(&s->mutex);
> > slot = s;
> > break;
>
> Uh. You're overwriting the existing pid here. Not good if the slot is
> currently in use.
>

Isn't that the point? We're acquiring the slot there, per the comment:

"Find a previously created slot and mark it as used by this backend."

> namecpy(&plugin, &slot->data.plugin);
> >
> > - active = slot->active;
> > + active_pid = slot->active_pid != 0;
>
> That doesn't look right.
>

No, that's certainly not right. I also could've sworn I sorted it out, but
that must've been another site, because sure enough it's still there.

I don't really like the 'pid' field for pg_replication_slots. About
> naming it 'active_in' or such?
>

It was originally named active_pid, but changed based on feedback from
others that 'pid' would be consistent with pg_stat_activity and
pg_replication_slots. I have no strong opinion on the name, though I'd
prefer it reflect that the field does in fact represent a process ID.

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