On 21 April 2015 at 15:19, Andres Freund <and...@anarazel.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
