Re: Gripes about walsender command processing

2020-09-16 Thread Tom Lane
Alvaro Herrera writes: > On 2020-Sep-16, Tom Lane wrote: >> Note that your changes need to be backpatched into v13, >> because AFAICS this code is violating the FE/BE protocol >> right now --- it's just luck that libpq isn't moaning >> about extra CommandComplete messages. But I don't think >>

Re: Gripes about walsender command processing

2020-09-16 Thread Alvaro Herrera
On 2020-Sep-16, Tom Lane wrote: > Note that your changes need to be backpatched into v13, > because AFAICS this code is violating the FE/BE protocol > right now --- it's just luck that libpq isn't moaning > about extra CommandComplete messages. But I don't think > we want to change the ps title

Re: Gripes about walsender command processing

2020-09-16 Thread Alvaro Herrera
On 2020-Sep-16, Tom Lane wrote: > Yeah, that works for me. It doesn't allow for having just one > set_ps_display() call ahead of the switch, but that isn't that > big a loss. We cannot merge the EndReplicationCommand calls to > after the switch, because some of the cases don't want one here; >

Re: Gripes about walsender command processing

2020-09-16 Thread Tom Lane
Alvaro Herrera writes: > On 2020-Sep-16, Tom Lane wrote: >> I don't see an easy way to improve on it though. The only obvious >> alternative would be to put another switch before the main one that >> just fills a "const char *cmdtag" variable, but that seems ugly. > The alternative of doing the

Re: Gripes about walsender command processing

2020-09-16 Thread Alvaro Herrera
On 2020-Sep-16, Tom Lane wrote: > This looks moderately reasonable to me. However, with the process > title reporting I want to add, we're going to end up with a switch > that looks like > > case T_IdentifySystemCmd: > + set_ps_display("IDENTIFY_SYSTEM"); >

Re: Gripes about walsender command processing

2020-09-16 Thread Tom Lane
Alvaro Herrera writes: > On 2020-Sep-15, Alvaro Herrera wrote: >> I overlooked this in 2f9661311b83. From this perspective, I agree it >> looks wrong. We still have to send *some* completion tag (the 'C' >> message), but maybe we can invent a separate entry point in dest.c for >> that --

Re: Gripes about walsender command processing

2020-09-16 Thread Alvaro Herrera
On 2020-Sep-15, Alvaro Herrera wrote: > I overlooked this in 2f9661311b83. From this perspective, I agree it > looks wrong. We still have to send *some* completion tag (the 'C' > message), but maybe we can invent a separate entry point in dest.c for > that -- EndReplicationCommand() or some

Re: Gripes about walsender command processing

2020-09-15 Thread Alvaro Herrera
On 2020-Sep-15, Tom Lane wrote: > [ blink ... ] So why is it that DropReplicationSlot does > > SetQueryCompletion(, CMDTAG_DROP_REPLICATION_SLOT, 0); > EndCommand(, DestRemote, false); > > when the caller will immediately after that do > > SetQueryCompletion(, CMDTAG_SELECT,

Re: Gripes about walsender command processing

2020-09-15 Thread Tom Lane
Michael Paquier writes: > On Mon, Sep 14, 2020 at 11:34:44PM -0400, Tom Lane wrote: >> (I don't quite follow your comment about repslot drop, btw.) > There is already a command tag equivalent to DROP_REPLICATION_SLOT: > $ git grep REPLICATION -- */cmdtaglist.h >

Re: Gripes about walsender command processing

2020-09-14 Thread Michael Paquier
On Mon, Sep 14, 2020 at 11:34:44PM -0400, Tom Lane wrote: > (I don't quite follow your comment about repslot drop, btw.) There is already a command tag equivalent to DROP_REPLICATION_SLOT: $ git grep REPLICATION -- */cmdtaglist.h

Re: Gripes about walsender command processing

2020-09-14 Thread Tom Lane
Michael Paquier writes: > On Mon, Sep 14, 2020 at 12:51:39PM -0400, Tom Lane wrote: >> (Is there a better way to get the name of a replication command? >> I didn't look very hard for one.) > Wouldn't that just be in cmdtaglist.h, but extended for nodes used for > replication commands? Then you

Re: Gripes about walsender command processing

2020-09-14 Thread Michael Paquier
On Mon, Sep 14, 2020 at 12:51:39PM -0400, Tom Lane wrote: > I looked into the other point about ps status always claiming the > walsender is "idle". This turns out to be something PostgresMain > does automatically, so unless we want to uglify that logic, we have > to make walsenders set the field

Re: Gripes about walsender command processing

2020-09-14 Thread Tom Lane
Michael Paquier writes: > On Sun, Sep 13, 2020 at 03:47:51PM -0400, Tom Lane wrote: >> While trying to fix this, I also observed that exec_replication_command >> fails to clean up the temp context it made for parsing the command string, >> if that turns out to be a SQL command. This very

Re: Gripes about walsender command processing

2020-09-14 Thread Michael Paquier
On Sun, Sep 13, 2020 at 03:47:51PM -0400, Tom Lane wrote: > While trying to fix this, I also observed that exec_replication_command > fails to clean up the temp context it made for parsing the command string, > if that turns out to be a SQL command. This very accidentally fails to > lead to a

Re: Gripes about walsender command processing

2020-09-13 Thread Tom Lane
I wrote: > I think that walsenders ought to report their current replication command > as though it were a SQL command. They oughta set debug_query_string, too. I also notice that a walsender does not maintain its ps status: postgres 921109 100 0.0 32568 11880 ?Rs 18:57 0:51

Gripes about walsender command processing

2020-09-13 Thread Tom Lane
While trying to understand a recent buildfarm failure [1], I got annoyed by the fact that a walsender exposes its last SQL command in pg_stat_activity even when that was a long time ago and what it's been doing since then is replication commands. This leads to *totally* misleading reports, for