Thread: Use array as object (src/fe_utils/parallel_slot.c)

On Fri, Aug 19, 2022 at 03:52:36PM -0300, Ranier Vilela wrote:
> Em qui., 11 de ago. de 2022 às 09:52, Ranier Vilela <ranier.vf@gmail.com> escreveu:
> 
> > Hi,
> >
> > One other case suspicious, which I think deserves a conference.
> > At function wait_on_slots (src/fe_utils/parallel_slot.c)
> > The variable "slots" are array, but at function call SetCancelConn,
> > "slots" are used as an object, which at the very least would be suspicious.
>
> The commit
> https://github.com/postgres/postgres/commit/f71519e545a34ece0a27c8bb1a2b6e197d323163
> Introduced the affected function.

It's true that the function was added there, but SetCancelConn() was called the
same way before that: SetCancelConn(slots->connection);

If you trace the history back to a17923204, you'll see a comment about the
"zeroth slot", which makes it clear that the first slot it what's intended.

I agree that it would be clearer if this were written as slots[0].connection.

-- 
Justin

On Fri, Aug 19, 2022 at 02:22:32PM -0500, Justin Pryzby wrote:
> If you trace the history back to a17923204, you'll see a comment about the
> "zeroth slot", which makes it clear that the first slot it what's intended.
>
> I agree that it would be clearer if this were written as slots[0].connection.

Based on the way the code is written on HEAD, this would be the
correct assumption.  Now, calling PQgetCancel() would return NULL for
a connection that we actually ignore in the code a couple of lines
above when it has PGINVALID_SOCKET.  So it seems to me that the
suggestion of using "cancelconn", which would be the first valid
connection, rather than always the first connection, which may be
using an invalid socket, is correct, so as we always have our hands
on a way to cancel a command.
--
Michael

On Fri, Aug 26, 2022 at 01:54:26PM -0300, Ranier Vilela wrote:
> Is it worth creating a commiffest?

Don't think so, but feel free to create one and mark me as committer
if you think that's appropriate.  I have marked this thread as
something to do soon-ishly, but I am being distracted by life this
month.
--
Michael

Michael Paquier <michael@paquier.xyz> writes:
> Based on the way the code is written on HEAD, this would be the
> correct assumption.  Now, calling PQgetCancel() would return NULL for
> a connection that we actually ignore in the code a couple of lines
> above when it has PGINVALID_SOCKET.  So it seems to me that the
> suggestion of using "cancelconn", which would be the first valid
> connection, rather than always the first connection, which may be
> using an invalid socket, is correct, so as we always have our hands
> on a way to cancel a command.

I came across this commit (52144b6fc) while writing release notes,
and I have to seriously question whether it's right yet.  The thing
that needs to be asked is, if we get a SIGINT in a program using this
logic, why would we propagate a cancel to just one of the controlled
sessions and not all of them?

It looks to me like the original concept was that slot zero would be
a "master" connection, such that canceling just that one would have a
useful effect.  Maybe the current users of parallel_slot.c still use
it like that, but I bet it's more likely that the connections are
all doing independent things and you really gotta cancel them all
if you want out.

I suppose maybe this commit improved matters: if you are running N jobs
then typing control-C N times (not too quickly) might eventually get
you out, by successively canceling the lowest-numbered surviving
connection.  Previously you could have pounded the key all day and
not gotten rid of any but the zero'th task.  OTOH, if the connections
don't exit but just go back to idle, which seems pretty possible,
then it's not clear we've moved the needle at all.

Anyway I think this needs rewritten, not just tweaked.  The cancel.c
infrastructure is really next to useless here since it is only designed
with one connection in mind.  I'm inclined to think we should only
expect the signal handler to set CancelRequested, and then manually
issue cancels to all live connections when we see that become set.

I'm not proposing reverting 52144b6fc, because I doubt it made
anything worse; but I'm thinking of leaving it out of the release
notes, because I'm unsure it had any user-visible effect at all.
It doesn't look to me like we'd ever get to wait_on_slots unless
all the connections are known busy.

            regards, tom lane