Re: PATCH: Add 'pid' column to pg_replication_slots

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.

> namecpy(&plugin, &slot->data.plugin);
>
> - active = slot->active;
> + active_pid = slot->active_pid != 0;

That doesn't look right.

> --- a/src/include/replication/slot.h
> +++ b/src/include/replication/slot.h
> @@ -84,13 +84,15 @@ typedef struct ReplicationSlot
> /* is this slot defined */
> bool in_use;
>
> - /* is somebody streaming out changes for this slot */
> - bool active;
> + /* field 'active' removed in 9.5; see 'active_pid' instead */
>
> /* any outstanding modifications? */
> bool just_dirtied;
> bool dirty;
>
> + /* Who is streaming out changes for this slot? 0 for nobody */
> + pid_t active_pid;
> +

That's a horrible idea. That way we end up with dozens of indirections
over time.

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

Other than these I plan to push this soon.

Greetings,

Andres Freund