On Tue, Feb 18, 2025 at 8:23 AM Michael Paquier <mich...@paquier.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.
v08-0001-Add-pipelining-support-in-psql.patch
Description: Binary data
v08-0002-Add-prompt-interpolation-and-variables-for-psql-.patch
Description: Binary data
