Re: Add Pipelining support in psql

On Tue, Feb 18, 2025 at 8:23 AM Michael Paquier <michael(at)paquier(dot)xyz> wrote:
> This is a pretty cool patch. I like the structure you have used for
> the integration with the tracking of the number of commands, the
> number of syncs (like pgbench) put in a pipeline, the number of
> results requested and the number of results available. That makes the
> whole easier to look at with a state in pset.

Thanks!

> + PSQL_SEND_PIPELINE_SYNC,
> + PSQL_START_PIPELINE_MODE,
> + PSQL_END_PIPELINE_MODE,
> + PSQL_FLUSH,
> + PSQL_SEND_FLUSH_REQUEST,
> + PSQL_GET_RESULTS,
>
> These new values are inconsistent, let's use some more PSQL_SEND_*
> here. That makes the whole set of values more greppable.

Changed.

> The tests in psql.sql are becoming really long. Perhaps it would be
> better to split that into its own file, say psql_pipeline.sql? The
> input file is already 2k lines, you are adding 15% more lines to that.

Agreed, I wasn't sure if this was enough to warrant a dedicated test
file. This is now separated in psql_pipeline.sql.

> + * otherwise, calling PQgetResults will block.
>
> Likely PQgetResults => PQgetResult().

Indeed, this is fixed.

> Set of nits with the style of the code, but I'd suggest to use
> braces {} here, to outline that the comment is in a block. There's a
> second, larger one in discardAbortedPipelineResults().
>
> + if (strcmp(cmd, "gx") == 0 && PQpipelineStatus(pset.db) != PQ_PIPELINE_OFF)
> + {
> + pg_log_error("\\gx not allowed in pipeline mode");
> + clean_extended_state();
> + return PSQL_CMD_ERROR;
> + }

Changed.

> What is the reasoning here behind this restriction? \gx is a wrapper
> of \g with expanded mode on, but it is also possible to call \g with
> expanded=on, bypassing this restriction.

The issue is that \gx enables expanded mode for the duration of the
query and immediately reset it in sendquery_cleanup. With pipelining,
the command is piped and displaying is done by either \endpipeline or
\getresults, so the flag change has no impact. Forbidding it was a way
to make it clearer that it won't have the expected effect. If we
wanted a similar feature, this would need to be done with something
like \endpipelinex or \getresultsx.

> Let's split the prompt patch with the support of %P into its own
> patch.
>
> -#define DEFAULT_PROMPT1 "%/%R%x%# "
> -#define DEFAULT_PROMPT2 "%/%R%x%# "
> +#define DEFAULT_PROMPT1 "%/%R%P%x%# "
> +#define DEFAULT_PROMPT2 "%/%R%P%x%# "
> #define DEFAULT_PROMPT3 ">> "
>
> I don't think that changing this default is a good idea. Everybody
> can do that in their own .psqlrc (spoiler: I do).
>
> The idea to use three fields with a hardcoded format does not look
> like a good idea to me. I think that this should be done in a
> different and more extensible way:
> - Use %P to track if we are in pipeline mode on, off or abort.
> - Define three new variables that behave like ROW_COUNT, but for the
> fields you want to track here. These could then be passed down to a
> PROMPT with %:name:, grammar already supported.
>
> That would make the whole much more flexible. At it seems to me that
> we could also add requested_results to this set? These could be named
> with the same prefix, like PIPELINE_SYNC_COUNT, etc.

I've split the patch and created the 3 special variables:
PIPELINE_SYNC_COUNT, PIPELINE_COMMAND_COUNT, PIPELINE_RESULT_COUNT.

For requested_results, I don't think there's value in exposing it
since it is used as an exit condition and thus will always be 0
outside of ExecQueryAndProcessResults.