Postgres Pro
Downloads
Home   >   mailing lists

Re: PATCH: Add 'pid' column to pg_replication_slots - Mailing list pgsql-hackers

From Craig Ringer
Subject Re: PATCH: Add 'pid' column to pg_replication_slots
Date
Msg-id CAMsr+YG03fakzadtLDH0ybQG3J6R+-bU9mkZ3zLmHjokk79h5w@mail.gmail.com
Whole thread Raw
In response to Re: PATCH: Add 'pid' column to pg_replication_slots  (Andres Freund <andres@anarazel.de>)
Responses Re: PATCH: Add 'pid' column to pg_replication_slots
Re: PATCH: Add 'pid' column to pg_replication_slots
List pgsql-hackers
Tree view


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

pgsql-hackers by date:

Previous
From: Etsuro Fujita
Date:
Subject: Update docs in fdwhandler.sgml
Next
From: Andres Freund
Date:
Subject: Re: PATCH: Add 'pid' column to pg_replication_slots