public inbox for [email protected]help / color / mirror / Atom feed
Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs 33+ messages / 14 participants [nested] [flat]
* Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs @ 2024-03-29 17:25 Laurenz Albe <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Laurenz Albe @ 2024-03-29 17:25 UTC (permalink / raw) To: Daniel Verite <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Jakub Wartak <[email protected]> On Fri, 2024-03-29 at 14:07 +0100, Laurenz Albe wrote: > I had a look at patch 0001 (0002 will follow). Here is the code review for patch number 2: > diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c [...] +static bool +SetupGOutput(PGresult *result, FILE **gfile_fout, bool *is_pipe) [...] +static void +CloseGOutput(FILE *gfile_fout, bool is_pipe) It makes sense to factor out this code. But shouldn't these functions have a prototype at the beginning of the file? > + /* > + * If FETCH_COUNT is set and the context allows it, use the single row > + * mode to fetch results and have no more than FETCH_COUNT rows in > + * memory. > + */ That comment talks about single-row mode, whey you are using chunked mode. You probably forgot to modify the comment from a previous version of the patch. > + if (fetch_count > 0 && !pset.crosstab_flag && !pset.gexec_flag && !is_watch > + && !pset.gset_prefix && pset.show_all_results) > + { > + /* > + * The row-by-chunks fetch is not enabled when SHOW_ALL_RESULTS is false, > + * since we would need to accumulate all rows before knowing > + * whether they need to be discarded or displayed, which contradicts > + * FETCH_COUNT. > + */ > + if (!PQsetChunkedRowsMode(pset.db, fetch_count)) > + { I think that comment should be before the "if" statement, not inside it. Here is a suggestion for a consolidated comment: Fetch the result in chunks if FETCH_COUNT is set. We don't enable chunking if SHOW_ALL_RESULTS is false, since that requires us to accumulate all rows before we can tell what should be displayed, which would counter the idea of FETCH_COUNT. Chunk fetching is also disabled if \gset, \crosstab, \gexec and \watch are used. > + if (fetch_count > 0 && result_status == PGRES_TUPLES_CHUNK) Could it be that result_status == PGRES_TUPLES_CHUNK, but fetch_count is 0? if not, perhaps there should be an Assert that verifies that, and the "if" statement should only check for the latter condition. > --- a/src/bin/psql/t/001_basic.pl > +++ b/src/bin/psql/t/001_basic.pl > @@ -184,10 +184,10 @@ like( > "\\set FETCH_COUNT 1\nSELECT error;\n\\errverbose", > on_error_stop => 0))[2], > qr/\A^psql:<stdin>:2: ERROR: .*$ > -^LINE 2: SELECT error;$ > +^LINE 1: SELECT error;$ > ^ *^.*$ > ^psql:<stdin>:3: error: ERROR: [0-9A-Z]{5}: .*$ > -^LINE 2: SELECT error;$ > +^LINE 1: SELECT error;$ Why does the output change? Perhaps there is a good and harmless explanation, but the naïve expectation would be that it doesn't. The patch does not apply any more because of a conflict with the non-blocking PQcancel patch. After fixing the problem manually, it builds without warning. The regression tests pass, and the feature works as expected. Yours, Laurenz Albe ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs @ 2024-04-01 17:52 Daniel Verite <[email protected]> parent: Laurenz Albe <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Daniel Verite @ 2024-04-01 17:52 UTC (permalink / raw) To: Laurenz Albe <[email protected]>; Laurenz Albe <[email protected]>; +Cc: pgsql-hackers; Tom Lane <[email protected]>; Robert Haas <[email protected]>; Jakub Wartak <[email protected]> Laurenz Albe wrote: > Here is the code review for patch number 2: > +static void > +CloseGOutput(FILE *gfile_fout, bool is_pipe) > > It makes sense to factor out this code. > But shouldn't these functions have a prototype at the beginning of the file? Looking at the other static functions in psql/common.c, there are 22 of them but only 3 have prototypes at the top of the file. These 3 functions are called before being defined, so these prototypes are mandatory. The other static functions that are defined before being called happen not to have forward declarations, so SetupGOutput() and CloseGOutput() follow that model. > Here is a suggestion for a consolidated comment: > > Fetch the result in chunks if FETCH_COUNT is set. We don't enable chunking > if SHOW_ALL_RESULTS is false, since that requires us to accumulate all rows > before we can tell what should be displayed, which would counter the idea > of FETCH_COUNT. Chunk fetching is also disabled if \gset, \crosstab, > \gexec and \watch are used. OK, done like that. > > + if (fetch_count > 0 && result_status == PGRES_TUPLES_CHUNK) > > Could it be that result_status == PGRES_TUPLES_CHUNK, but fetch_count is 0? > if not, perhaps there should be an Assert that verifies that, and the "if" > statement should only check for the latter condition. Good point. In fact it can be simplified to if (result_status == PGRES_TUPLES_CHUNK), and fetch_count as a variable can be removed from the function. Done that way. > > --- a/src/bin/psql/t/001_basic.pl > > +++ b/src/bin/psql/t/001_basic.pl > > @@ -184,10 +184,10 @@ like( > > "\\set FETCH_COUNT 1\nSELECT error;\n\\errverbose", > > on_error_stop => 0))[2], > > qr/\A^psql:<stdin>:2: ERROR: .*$ > > -^LINE 2: SELECT error;$ > > +^LINE 1: SELECT error;$ > > ^ *^.*$ > > ^psql:<stdin>:3: error: ERROR: [0-9A-Z]{5}: .*$ > > -^LINE 2: SELECT error;$ > > +^LINE 1: SELECT error;$ > > Why does the output change? Perhaps there is a good and harmless > explanation, but the naïve expectation would be that it doesn't. Unpatched, psql builds this query: DECLARE _psql_cursor NO SCROLL CURSOR FOR \n <user-query> therefore the user query starts at line 2. With the patch, the user query is sent as-is, starting at line1, hence the different error location. > After fixing the problem manually, it builds without warning. > The regression tests pass, and the feature works as expected. Thanks for testing. Updated patches are attached. Best regards, -- Daniel Vérité https://postgresql.verite.pro/ Twitter: @DanielVerite From 0fefaee7d5b3003ad0d089ea9e92675c6f50245f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20V=C3=A9rit=C3=A9?= <[email protected]> Date: Mon, 1 Apr 2024 19:46:20 +0200 Subject: [PATCH v7 1/2] Implement retrieval of results in chunks with libpq. This mode is similar to the single-row mode except that chunks of results contain up to N rows instead of a single row. It is meant to reduce the overhead of the row-by-row allocations for large result sets. The mode is selected with PQsetChunkedRowsMode(int maxRows) and results have the new status code PGRES_TUPLES_CHUNK. --- doc/src/sgml/libpq.sgml | 98 +++++++++++---- .../libpqwalreceiver/libpqwalreceiver.c | 1 + src/bin/pg_amcheck/pg_amcheck.c | 1 + src/interfaces/libpq/exports.txt | 1 + src/interfaces/libpq/fe-exec.c | 117 +++++++++++++++--- src/interfaces/libpq/libpq-fe.h | 4 +- src/interfaces/libpq/libpq-int.h | 7 +- 7 files changed, 185 insertions(+), 44 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index d3e87056f2..1814921d5a 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -3545,7 +3545,20 @@ ExecStatusType PQresultStatus(const PGresult *res); The <structname>PGresult</structname> contains a single result tuple from the current command. This status occurs only when single-row mode has been selected for the query - (see <xref linkend="libpq-single-row-mode"/>). + (see <xref linkend="libpq-chunked-results-modes"/>). + </para> + </listitem> + </varlistentry> + + <varlistentry id="libpq-pgres-tuples-chunk"> + <term><literal>PGRES_TUPLES_CHUNK</literal></term> + <listitem> + <para> + The <structname>PGresult</structname> contains several tuples + from the current command. The count of tuples cannot exceed + the maximum passed to <xref linkend="libpq-PQsetChunkedRowsMode"/>. + This status occurs only when the chunked mode has been selected + for the query (see <xref linkend="libpq-chunked-results-modes"/>). </para> </listitem> </varlistentry> @@ -5197,8 +5210,8 @@ PGresult *PQgetResult(PGconn *conn); <para> Another frequently-desired feature that can be obtained with <xref linkend="libpq-PQsendQuery"/> and <xref linkend="libpq-PQgetResult"/> - is retrieving large query results a row at a time. This is discussed - in <xref linkend="libpq-single-row-mode"/>. + is retrieving large query results a limited number of rows at a time. This is discussed + in <xref linkend="libpq-chunked-results-modes"/>. </para> <para> @@ -5562,12 +5575,13 @@ int PQflush(PGconn *conn); </para> <para> - To enter single-row mode, call <function>PQsetSingleRowMode</function> - before retrieving results with <function>PQgetResult</function>. - This mode selection is effective only for the query currently - being processed. For more information on the use of - <function>PQsetSingleRowMode</function>, - refer to <xref linkend="libpq-single-row-mode"/>. + To enter single-row or chunked modes, call + respectively <function>PQsetSingleRowMode</function> + or <function>PQsetChunkedRowsMode</function> before retrieving results + with <function>PQgetResult</function>. This mode selection is effective + only for the query currently being processed. For more information on the + use of these functions refer + to <xref linkend="libpq-chunked-results-modes" />. </para> <para> @@ -5934,10 +5948,10 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; </sect2> </sect1> - <sect1 id="libpq-single-row-mode"> - <title>Retrieving Query Results Row-by-Row</title> + <sect1 id="libpq-chunked-results-modes"> + <title>Retrieving Query Results in chunks</title> - <indexterm zone="libpq-single-row-mode"> + <indexterm zone="libpq-chunked-results-modes"> <primary>libpq</primary> <secondary>single-row mode</secondary> </indexterm> @@ -5948,13 +5962,15 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; <structname>PGresult</structname>. This can be unworkable for commands that return a large number of rows. For such cases, applications can use <xref linkend="libpq-PQsendQuery"/> and <xref linkend="libpq-PQgetResult"/> in - <firstterm>single-row mode</firstterm>. In this mode, the result row(s) are - returned to the application one at a time, as they are received from the - server. + <firstterm>single-row mode</firstterm> or <firstterm>chunked mode</firstterm>. + In these modes, the result row(s) are returned to the application one at a + time for the single-row mode and by chunks for the chunked mode, as they + are received from the server. </para> <para> - To enter single-row mode, call <xref linkend="libpq-PQsetSingleRowMode"/> + To enter these modes, call <xref linkend="libpq-PQsetSingleRowMode"/> + or <xref linkend="libpq-PQsetChunkedRowsMode"/> immediately after a successful call of <xref linkend="libpq-PQsendQuery"/> (or a sibling function). This mode selection is effective only for the currently executing query. Then call <xref linkend="libpq-PQgetResult"/> @@ -5962,7 +5978,8 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; linkend="libpq-async"/>. If the query returns any rows, they are returned as individual <structname>PGresult</structname> objects, which look like normal query results except for having status code - <literal>PGRES_SINGLE_TUPLE</literal> instead of + <literal>PGRES_SINGLE_TUPLE</literal> for the single-row mode and + <literal>PGRES_TUPLES_CHUNK</literal> for the chunked mode, instead of <literal>PGRES_TUPLES_OK</literal>. After the last row, or immediately if the query returns zero rows, a zero-row object with status <literal>PGRES_TUPLES_OK</literal> is returned; this is the signal that no @@ -5975,9 +5992,9 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; </para> <para> - When using pipeline mode, single-row mode needs to be activated for each - query in the pipeline before retrieving results for that query - with <function>PQgetResult</function>. + When using pipeline mode, the single-row or chunked mode need to be + activated for each query in the pipeline before retrieving results for that + query with <function>PQgetResult</function>. See <xref linkend="libpq-pipeline-mode"/> for more information. </para> @@ -6011,14 +6028,49 @@ int PQsetSingleRowMode(PGconn *conn); </variablelist> </para> + <para> + <variablelist> + <varlistentry id="libpq-PQsetChunkedRowsMode"> + <term><function>PQsetChunkedRowsMode</function> + <indexterm><primary>PQsetChunkedRowsMode</primary></indexterm></term> + <listitem> + <para> + Select to receive the results for the currently-executing query in chunks. + +<synopsis> + int PQsetChunkedRowsMode(PGconn *conn, + int maxRows); +</synopsis> + </para> + + <para> + This function is similar to <xref linkend="libpq-PQsetSingleRowMode"/>, + except that it can retrieve <replaceable>maxRows</replaceable> rows + per call to <xref linkend="libpq-PQgetResult"/> instead of a single row. + This function can only be called immediately after + <xref linkend="libpq-PQsendQuery"/> or one of its sibling functions, + before any other operation on the connection such as + <xref linkend="libpq-PQconsumeInput"/> or + <xref linkend="libpq-PQgetResult"/>. If called at the correct time, + the function activates the chunked mode for the current query and + returns 1. Otherwise the mode stays unchanged and the function + returns 0. In any case, the mode reverts to normal after + completion of the current query. + </para> + </listitem> + </varlistentry> + </variablelist> + </para> + <caution> <para> While processing a query, the server may return some rows and then encounter an error, causing the query to be aborted. Ordinarily, <application>libpq</application> discards any such rows and reports only the - error. But in single-row mode, those rows will have already been - returned to the application. Hence, the application will see some - <literal>PGRES_SINGLE_TUPLE</literal> <structname>PGresult</structname> + error. But in the single-row or chunked modes, those rows will have already + been returned to the application. Hence, the application will see some + <literal>PGRES_SINGLE_TUPLE</literal> or <literal>PGRES_TUPLES_CHUNK</literal> + <structname>PGresult</structname> objects followed by a <literal>PGRES_FATAL_ERROR</literal> object. For proper transactional behavior, the application must be designed to discard or undo whatever has been done with the previously-processed diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c index 761bf0f677..83a465a390 100644 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c @@ -1249,6 +1249,7 @@ libpqrcv_exec(WalReceiverConn *conn, const char *query, switch (PQresultStatus(pgres)) { case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: case PGRES_TUPLES_OK: walres->status = WALRCV_OK_TUPLES; libpqrcv_processTuples(pgres, walres, nRetTypes, retTypes); diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index e5f9eedc47..728305a7cf 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -989,6 +989,7 @@ should_processing_continue(PGresult *res) case PGRES_COPY_IN: case PGRES_COPY_BOTH: case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: case PGRES_PIPELINE_SYNC: case PGRES_PIPELINE_ABORTED: return false; diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index 9fbd3d3407..c7d01958ab 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -202,3 +202,4 @@ PQcancelSocket 199 PQcancelErrorMessage 200 PQcancelReset 201 PQcancelFinish 202 +PQsetChunkedRowsMode 203 diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index c02a9180b2..b9a73b583e 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -41,7 +41,8 @@ char *const pgresStatus[] = { "PGRES_COPY_BOTH", "PGRES_SINGLE_TUPLE", "PGRES_PIPELINE_SYNC", - "PGRES_PIPELINE_ABORTED" + "PGRES_PIPELINE_ABORTED", + "PGRES_TUPLES_CHUNK" }; /* We return this if we're unable to make a PGresult at all */ @@ -83,7 +84,7 @@ static int check_field_number(const PGresult *res, int field_num); static void pqPipelineProcessQueue(PGconn *conn); static int pqPipelineSyncInternal(PGconn *conn, bool immediate_flush); static int pqPipelineFlush(PGconn *conn); - +static bool canChangeRowMode(PGconn *conn); /* ---------------- * Space management for PGresult. @@ -200,6 +201,7 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) case PGRES_COPY_IN: case PGRES_COPY_BOTH: case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: /* non-error cases */ break; default: @@ -913,8 +915,9 @@ pqPrepareAsyncResult(PGconn *conn) /* * Replace conn->result with next_result, if any. In the normal case * there isn't a next result and we're just dropping ownership of the - * current result. In single-row mode this restores the situation to what - * it was before we created the current single-row result. + * current result. In single-row and chunked modes this restores the + * situation to what it was before we created the current single-row or + * chunk-of-rows result. */ conn->result = conn->next_result; conn->error_result = false; /* next_result is never an error */ @@ -1200,10 +1203,11 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value) * (Such a string should already be translated via libpq_gettext().) * If it is left NULL, the error is presumed to be "out of memory". * - * In single-row mode, we create a new result holding just the current row, - * stashing the previous result in conn->next_result so that it becomes - * active again after pqPrepareAsyncResult(). This allows the result metadata - * (column descriptions) to be carried forward to each result row. + * In single-row or chunked mode, we create a new result holding just the + * current set of rows, stashing the previous result in conn->next_result so + * that it becomes active again after pqPrepareAsyncResult(). This allows the + * result metadata (column descriptions) to be carried forward to each result + * row. */ int pqRowProcessor(PGconn *conn, const char **errmsgp) @@ -1228,6 +1232,28 @@ pqRowProcessor(PGconn *conn, const char **errmsgp) if (!res) return 0; } + else if (conn->rowsChunkSize > 0) + { + /* + * In chunked mode, make a new PGresult that will hold N rows; the + * original conn->result is left unchanged, as in the single-row mode. + */ + if (!conn->chunk_result) + { + /* Allocate and initialize the result to hold a chunk of rows */ + res = PQcopyResult(res, + PG_COPYRES_ATTRS | PG_COPYRES_EVENTS | + PG_COPYRES_NOTICEHOOKS); + if (!res) + return 0; + /* Change result status to special chunk-of-rows value */ + res->resultStatus = PGRES_TUPLES_CHUNK; + /* Keep this result to reuse for the next rows of the chunk */ + conn->chunk_result = res; + } + else + res = conn->chunk_result; /* Use the current chunk */ + } /* * Basically we just allocate space in the PGresult for each field and @@ -1290,6 +1316,21 @@ pqRowProcessor(PGconn *conn, const char **errmsgp) conn->asyncStatus = PGASYNC_READY_MORE; } + /* + * In chunked mode, if the count has reached the requested limit, make the + * rows of the current chunk available immediately. + */ + else if (conn->rowsChunkSize > 0 && res->ntups >= conn->rowsChunkSize) + { + /* Stash old result for re-use later */ + conn->next_result = conn->result; + conn->result = res; + /* Do not reuse that chunk of results */ + conn->chunk_result = NULL; + /* And mark the result ready to return */ + conn->asyncStatus = PGASYNC_READY_MORE; + } + return 1; fail: @@ -1745,8 +1786,9 @@ PQsendQueryStart(PGconn *conn, bool newQuery) */ pqClearAsyncResult(conn); - /* reset single-row processing mode */ + /* reset row-by-row and chunked processing modes */ conn->singleRowMode = false; + conn->rowsChunkSize = 0; } /* ready to send command message */ @@ -1930,25 +1972,51 @@ sendFailed: */ int PQsetSingleRowMode(PGconn *conn) +{ + if (canChangeRowMode(conn)) + { + conn->singleRowMode = true; + return 1; + } + else + return 0; +} + +/* + * Select chunked results processing mode + */ +int +PQsetChunkedRowsMode(PGconn *conn, int chunkSize) +{ + if (chunkSize >= 0 && canChangeRowMode(conn)) + { + conn->rowsChunkSize = chunkSize; + return 1; + } + else + return 0; +} + +static +bool +canChangeRowMode(PGconn *conn) { /* - * Only allow setting the flag when we have launched a query and not yet - * received any results. + * Only allow setting the row-by-row or by-chunks modes when we have + * launched a query and not yet received any results. */ if (!conn) - return 0; + return false; if (conn->asyncStatus != PGASYNC_BUSY) - return 0; + return false; if (!conn->cmd_queue_head || (conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE && conn->cmd_queue_head->queryclass != PGQUERY_EXTENDED)) - return 0; + return false; if (pgHavePendingResult(conn)) - return 0; + return false; - /* OK, set flag */ - conn->singleRowMode = true; - return 1; + return true; } /* @@ -2115,6 +2183,16 @@ PQgetResult(PGconn *conn) break; case PGASYNC_READY: + /* + * If there is a pending chunk of results, return it + */ + if (conn->chunk_result != NULL) + { + res = conn->chunk_result; + conn->chunk_result = NULL; + break; + } + res = pqPrepareAsyncResult(conn); /* Advance the queue as appropriate */ @@ -3173,10 +3251,11 @@ pqPipelineProcessQueue(PGconn *conn) } /* - * Reset single-row processing mode. (Client has to set it up for each + * Reset to full result sets mode. (Client has to set it up for each * query, if desired.) */ conn->singleRowMode = false; + conn->rowsChunkSize = 0; /* * If there are no further commands to process in the queue, get us in diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index 09b485bd2b..0cea4f6b5b 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -112,8 +112,9 @@ typedef enum PGRES_COPY_BOTH, /* Copy In/Out data transfer in progress */ PGRES_SINGLE_TUPLE, /* single tuple from larger resultset */ PGRES_PIPELINE_SYNC, /* pipeline synchronization point */ - PGRES_PIPELINE_ABORTED /* Command didn't run because of an abort + PGRES_PIPELINE_ABORTED, /* Command didn't run because of an abort * earlier in a pipeline */ + PGRES_TUPLES_CHUNK /* set of tuples from larger resultset */ } ExecStatusType; typedef enum @@ -489,6 +490,7 @@ extern int PQsendQueryPrepared(PGconn *conn, const int *paramFormats, int resultFormat); extern int PQsetSingleRowMode(PGconn *conn); +extern int PQsetChunkedRowsMode(PGconn *conn, int chunkSize); extern PGresult *PQgetResult(PGconn *conn); /* Routines for managing an asynchronous query */ diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 9c05f11a6e..23c7a399ab 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -435,6 +435,8 @@ struct pg_conn * sending semantics */ PGpipelineStatus pipelineStatus; /* status of pipeline mode */ bool singleRowMode; /* return current query result row-by-row? */ + int rowsChunkSize; /* non-zero to return query results by chunks + * not exceeding that number of rows */ char copy_is_binary; /* 1 = copy binary, 0 = copy text */ int copy_already_done; /* # bytes already returned in COPY OUT */ PGnotify *notifyHead; /* oldest unreported Notify msg */ @@ -540,7 +542,10 @@ struct pg_conn */ PGresult *result; /* result being constructed */ bool error_result; /* do we need to make an ERROR result? */ - PGresult *next_result; /* next result (used in single-row mode) */ + PGresult *next_result; /* next result (used in single-row and + * by-chunks modes) */ + PGresult *chunk_result; /* current chunk of results (limited to + * rowsChunkSize) */ /* Assorted state for SASL, SSL, GSS, etc */ const pg_fe_sasl_mech *sasl; -- 2.34.1 From 33f043aeaf3969e66f6c80af6ef6ea27499b4740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20V=C3=A9rit=C3=A9?= <[email protected]> Date: Mon, 1 Apr 2024 19:46:42 +0200 Subject: [PATCH v7 2/2] Reimplement FETCH_COUNT with the chunked mode in libpq. Cursors were used only when the command starts with the keyword "SELECT", excluding queries that start with "WITH" or "UPDATE" or "INSERT" that may also return large result sets. --- src/bin/psql/common.c | 538 ++++++++++------------------- src/bin/psql/t/001_basic.pl | 6 +- src/test/regress/expected/psql.out | 9 +- src/test/regress/sql/psql.sql | 4 +- 4 files changed, 189 insertions(+), 368 deletions(-) diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 2830bde495..2112e1a423 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -31,7 +31,6 @@ #include "settings.h" static bool DescribeQuery(const char *query, double *elapsed_msec); -static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec); static int ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_gone_p, @@ -40,8 +39,6 @@ static int ExecQueryAndProcessResults(const char *query, const printQueryOpt *opt, FILE *printQueryFout); static bool command_no_begin(const char *query); -static bool is_select_command(const char *query); - /* * openQueryOutputFile --- attempt to open a query output file @@ -373,6 +370,7 @@ AcceptResult(const PGresult *result, bool show_error) { case PGRES_COMMAND_OK: case PGRES_TUPLES_OK: + case PGRES_TUPLES_CHUNK: case PGRES_EMPTY_QUERY: case PGRES_COPY_IN: case PGRES_COPY_OUT: @@ -1135,16 +1133,10 @@ SendQuery(const char *query) /* Describe query's result columns, without executing it */ OK = DescribeQuery(query, &elapsed_msec); } - else if (pset.fetch_count <= 0 || pset.gexec_flag || - pset.crosstab_flag || !is_select_command(query)) - { - /* Default fetch-it-all-and-print mode */ - OK = (ExecQueryAndProcessResults(query, &elapsed_msec, &svpt_gone, false, 0, NULL, NULL) > 0); - } else { - /* Fetch-in-segments mode */ - OK = ExecQueryUsingCursor(query, &elapsed_msec); + /* Default fetch-and-print mode */ + OK = (ExecQueryAndProcessResults(query, &elapsed_msec, &svpt_gone, false, 0, NULL, NULL) > 0); } if (!OK && pset.echo == PSQL_ECHO_ERRORS) @@ -1396,6 +1388,47 @@ DescribeQuery(const char *query, double *elapsed_msec) return OK; } +/* + * Check if an output stream for \g needs to be opened, and if + * yes, open it. + * Return false if an error occurred, true otherwise. + */ +static bool +SetupGOutput(PGresult *result, FILE **gfile_fout, bool *is_pipe) +{ + ExecStatusType status = PQresultStatus(result); + if (pset.gfname != NULL && /* there is a \g file or program */ + *gfile_fout == NULL && /* and it's not already opened */ + (status == PGRES_TUPLES_OK || + status == PGRES_TUPLES_CHUNK || + status == PGRES_COPY_OUT)) + { + if (openQueryOutputFile(pset.gfname, gfile_fout, is_pipe)) + { + if (is_pipe) + disable_sigpipe_trap(); + } + else + return false; + } + return true; +} + +static void +CloseGOutput(FILE *gfile_fout, bool is_pipe) +{ + /* close \g file if we opened it */ + if (gfile_fout) + { + if (is_pipe) + { + SetShellResultVariables(pclose(gfile_fout)); + restore_sigpipe_trap(); + } + else + fclose(gfile_fout); + } +} /* * ExecQueryAndProcessResults: utility function for use by SendQuery() @@ -1429,9 +1462,14 @@ ExecQueryAndProcessResults(const char *query, instr_time before, after; PGresult *result; + FILE *gfile_fout = NULL; bool gfile_is_pipe = false; + int64 total_tuples = 0; + int flush_error = 0; + bool is_pager = false; + if (timing) INSTR_TIME_SET_CURRENT(before); else @@ -1454,6 +1492,23 @@ ExecQueryAndProcessResults(const char *query, return -1; } + /* + * Fetch the result in chunks if FETCH_COUNT is set. + * We don't enable chunking if SHOW_ALL_RESULTS is false, since that + * requires us to accumulate all rows before we can tell what should be + * displayed, which would counter the idea of FETCH_COUNT. + * Chunk fetching is also disabled if \gset, \crosstab, \gexec and \watch + * are used. + */ + if (pset.fetch_count > 0 && !pset.crosstab_flag && !pset.gexec_flag && !is_watch + && !pset.gset_prefix && pset.show_all_results) + { + if (!PQsetChunkedRowsMode(pset.db, pset.fetch_count)) + { + pg_log_warning("fetching results in chunks mode is unavailable"); + } + } + /* * If SIGINT is sent while the query is processing, the interrupt will be * consumed. The user's intention, though, is to cancel the entire watch @@ -1477,6 +1532,8 @@ ExecQueryAndProcessResults(const char *query, ExecStatusType result_status; PGresult *next_result; bool last; + /* whether the output starts before results are fully fetched */ + bool partial_display = false; if (!AcceptResult(result, false)) { @@ -1572,20 +1629,9 @@ ExecQueryAndProcessResults(const char *query, } else if (pset.gfname) { - /* send to \g file, which we may have opened already */ - if (gfile_fout == NULL) - { - if (openQueryOutputFile(pset.gfname, - &gfile_fout, &gfile_is_pipe)) - { - if (gfile_is_pipe) - disable_sigpipe_trap(); - copy_stream = gfile_fout; - } - else - success = false; - } - else + /* COPY followed by \g filename or \g |program */ + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (success) copy_stream = gfile_fout; } else @@ -1603,6 +1649,90 @@ ExecQueryAndProcessResults(const char *query, success &= HandleCopyResult(&result, copy_stream); } + if (result_status == PGRES_TUPLES_CHUNK) + { + FILE *tuples_fout = printQueryFout ? printQueryFout : stdout; + printQueryOpt my_popt = pset.popt; + + total_tuples = 0; + partial_display = true; + + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (gfile_fout) + tuples_fout = gfile_fout; + + /* initialize print options for partial table output */ + my_popt.topt.start_table = true; + my_popt.topt.stop_table = false; + my_popt.topt.prior_records = 0; + + while (success) + { + /* pager: open at most once per resultset */ + if (tuples_fout == stdout && !is_pager) + { + tuples_fout = PageOutput(INT_MAX, &(my_popt.topt)); + is_pager = true; + } + /* display the current chunk of results unless the output stream is not working */ + if (!flush_error) + { + printQuery(result, &my_popt, tuples_fout, is_pager, pset.logfile); + flush_error = fflush(tuples_fout); + } + + /* after the first result set, disallow header decoration */ + my_popt.topt.start_table = false; + my_popt.topt.prior_records += PQntuples(result); + total_tuples += PQntuples(result); + + ClearOrSaveResult(result); + + result = PQgetResult(pset.db); + if (result == NULL) + { + /* + * Error. We expect a PGRES_TUPLES_OK result with + * zero tuple in it to finish the fetch sequence. + */ + success = false; + if (is_pager) + ClosePager(tuples_fout); + break; + } + else if (PQresultStatus(result) == PGRES_TUPLES_OK) + { + /* + * The last row has been read. Display the footer. + */ + my_popt.topt.stop_table = true; + printQuery(result, &my_popt, tuples_fout, is_pager, pset.logfile); + total_tuples += PQntuples(result); + + if (is_pager) + ClosePager(tuples_fout); + ClearOrSaveResult(result); + result = NULL; + break; + } + else if (PQresultStatus(result) != PGRES_TUPLES_CHUNK) + { + /* + * Error. We expect either PGRES_TUPLES_CHUNK or + * PGRES_TUPLES_OK. + */ + if (is_pager) + ClosePager(tuples_fout); + success = false; + AcceptResult(result, true); /* display error whenever appropriate */ + SetResultVariables(result, success); + break; + } + } + } + else + partial_display = false; + /* * Check PQgetResult() again. In the typical case of a single-command * string, it will return NULL. Otherwise, we'll have other results @@ -1631,7 +1761,7 @@ ExecQueryAndProcessResults(const char *query, } /* this may or may not print something depending on settings */ - if (result != NULL) + if (result != NULL && !partial_display) { /* * If results need to be printed into the file specified by \g, @@ -1640,32 +1770,33 @@ ExecQueryAndProcessResults(const char *query, * tuple output, but it's still used for status output. */ FILE *tuples_fout = printQueryFout; - bool do_print = true; - - if (PQresultStatus(result) == PGRES_TUPLES_OK && - pset.gfname) - { - if (gfile_fout == NULL) - { - if (openQueryOutputFile(pset.gfname, - &gfile_fout, &gfile_is_pipe)) - { - if (gfile_is_pipe) - disable_sigpipe_trap(); - } - else - success = do_print = false; - } + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (gfile_fout) tuples_fout = gfile_fout; - } - if (do_print) + if (success) success &= PrintQueryResult(result, last, opt, tuples_fout, printQueryFout); } /* set variables from last result */ if (!is_watch && last) - SetResultVariables(result, success); + { + if (!partial_display) + SetResultVariables(result, success); + else if (success) + { + /* + * fake SetResultVariables(). If an error occurred when + * retrieving chunks, these variables have been set already. + */ + char buf[32]; + + SetVariable(pset.vars, "ERROR", "false"); + SetVariable(pset.vars, "SQLSTATE", "00000"); + snprintf(buf, sizeof(buf), INT64_FORMAT, total_tuples); + SetVariable(pset.vars, "ROW_COUNT", buf); + } + } ClearOrSaveResult(result); result = next_result; @@ -1677,17 +1808,7 @@ ExecQueryAndProcessResults(const char *query, } } - /* close \g file if we opened it */ - if (gfile_fout) - { - if (gfile_is_pipe) - { - SetShellResultVariables(pclose(gfile_fout)); - restore_sigpipe_trap(); - } - else - fclose(gfile_fout); - } + CloseGOutput(gfile_fout, gfile_is_pipe); /* may need this to recover from conn loss during COPY */ if (!CheckConnection()) @@ -1700,274 +1821,6 @@ ExecQueryAndProcessResults(const char *query, } -/* - * ExecQueryUsingCursor: run a SELECT-like query using a cursor - * - * This feature allows result sets larger than RAM to be dealt with. - * - * Returns true if the query executed successfully, false otherwise. - * - * If pset.timing is on, total query time (exclusive of result-printing) is - * stored into *elapsed_msec. - */ -static bool -ExecQueryUsingCursor(const char *query, double *elapsed_msec) -{ - bool OK = true; - PGresult *result; - PQExpBufferData buf; - printQueryOpt my_popt = pset.popt; - bool timing = pset.timing; - FILE *fout; - bool is_pipe; - bool is_pager = false; - bool started_txn = false; - int64 total_tuples = 0; - int ntuples; - int fetch_count; - char fetch_cmd[64]; - instr_time before, - after; - int flush_error; - - *elapsed_msec = 0; - - /* initialize print options for partial table output */ - my_popt.topt.start_table = true; - my_popt.topt.stop_table = false; - my_popt.topt.prior_records = 0; - - if (timing) - INSTR_TIME_SET_CURRENT(before); - else - INSTR_TIME_SET_ZERO(before); - - /* if we're not in a transaction, start one */ - if (PQtransactionStatus(pset.db) == PQTRANS_IDLE) - { - result = PQexec(pset.db, "BEGIN"); - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - if (!OK) - return false; - started_txn = true; - } - - /* Send DECLARE CURSOR */ - initPQExpBuffer(&buf); - appendPQExpBuffer(&buf, "DECLARE _psql_cursor NO SCROLL CURSOR FOR\n%s", - query); - - result = PQexec(pset.db, buf.data); - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - if (!OK) - SetResultVariables(result, OK); - ClearOrSaveResult(result); - termPQExpBuffer(&buf); - if (!OK) - goto cleanup; - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - /* - * In \gset mode, we force the fetch count to be 2, so that we will throw - * the appropriate error if the query returns more than one row. - */ - if (pset.gset_prefix) - fetch_count = 2; - else - fetch_count = pset.fetch_count; - - snprintf(fetch_cmd, sizeof(fetch_cmd), - "FETCH FORWARD %d FROM _psql_cursor", - fetch_count); - - /* prepare to write output to \g argument, if any */ - if (pset.gfname) - { - if (!openQueryOutputFile(pset.gfname, &fout, &is_pipe)) - { - OK = false; - goto cleanup; - } - if (is_pipe) - disable_sigpipe_trap(); - } - else - { - fout = pset.queryFout; - is_pipe = false; /* doesn't matter */ - } - - /* clear any pre-existing error indication on the output stream */ - clearerr(fout); - - for (;;) - { - if (timing) - INSTR_TIME_SET_CURRENT(before); - - /* get fetch_count tuples at a time */ - result = PQexec(pset.db, fetch_cmd); - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - if (PQresultStatus(result) != PGRES_TUPLES_OK) - { - /* shut down pager before printing error message */ - if (is_pager) - { - ClosePager(fout); - is_pager = false; - } - - OK = AcceptResult(result, true); - Assert(!OK); - SetResultVariables(result, OK); - ClearOrSaveResult(result); - break; - } - - if (pset.gset_prefix) - { - /* StoreQueryTuple will complain if not exactly one row */ - OK = StoreQueryTuple(result); - ClearOrSaveResult(result); - break; - } - - /* - * Note we do not deal with \gdesc, \gexec or \crosstabview modes here - */ - - ntuples = PQntuples(result); - total_tuples += ntuples; - - if (ntuples < fetch_count) - { - /* this is the last result set, so allow footer decoration */ - my_popt.topt.stop_table = true; - } - else if (fout == stdout && !is_pager) - { - /* - * If query requires multiple result sets, hack to ensure that - * only one pager instance is used for the whole mess - */ - fout = PageOutput(INT_MAX, &(my_popt.topt)); - is_pager = true; - } - - printQuery(result, &my_popt, fout, is_pager, pset.logfile); - - ClearOrSaveResult(result); - - /* after the first result set, disallow header decoration */ - my_popt.topt.start_table = false; - my_popt.topt.prior_records += ntuples; - - /* - * Make sure to flush the output stream, so intermediate results are - * visible to the client immediately. We check the results because if - * the pager dies/exits/etc, there's no sense throwing more data at - * it. - */ - flush_error = fflush(fout); - - /* - * Check if we are at the end, if a cancel was pressed, or if there - * were any errors either trying to flush out the results, or more - * generally on the output stream at all. If we hit any errors - * writing things to the stream, we presume $PAGER has disappeared and - * stop bothering to pull down more data. - */ - if (ntuples < fetch_count || cancel_pressed || flush_error || - ferror(fout)) - break; - } - - if (pset.gfname) - { - /* close \g argument file/pipe */ - if (is_pipe) - { - SetShellResultVariables(pclose(fout)); - restore_sigpipe_trap(); - } - else - fclose(fout); - } - else if (is_pager) - { - /* close transient pager */ - ClosePager(fout); - } - - if (OK) - { - /* - * We don't have a PGresult here, and even if we did it wouldn't have - * the right row count, so fake SetResultVariables(). In error cases, - * we already set the result variables above. - */ - char buf[32]; - - SetVariable(pset.vars, "ERROR", "false"); - SetVariable(pset.vars, "SQLSTATE", "00000"); - snprintf(buf, sizeof(buf), INT64_FORMAT, total_tuples); - SetVariable(pset.vars, "ROW_COUNT", buf); - } - -cleanup: - if (timing) - INSTR_TIME_SET_CURRENT(before); - - /* - * We try to close the cursor on either success or failure, but on failure - * ignore the result (it's probably just a bleat about being in an aborted - * transaction) - */ - result = PQexec(pset.db, "CLOSE _psql_cursor"); - if (OK) - { - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - } - else - PQclear(result); - - if (started_txn) - { - result = PQexec(pset.db, OK ? "COMMIT" : "ROLLBACK"); - OK &= AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - } - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - return OK; -} - - /* * Advance the given char pointer over white space and SQL comments. */ @@ -2247,43 +2100,6 @@ command_no_begin(const char *query) } -/* - * Check whether the specified command is a SELECT (or VALUES). - */ -static bool -is_select_command(const char *query) -{ - int wordlen; - - /* - * First advance over any whitespace, comments and left parentheses. - */ - for (;;) - { - query = skip_white_space(query); - if (query[0] == '(') - query++; - else - break; - } - - /* - * Check word length (since "selectx" is not "select"). - */ - wordlen = 0; - while (isalpha((unsigned char) query[wordlen])) - wordlen += PQmblenBounded(&query[wordlen], pset.encoding); - - if (wordlen == 6 && pg_strncasecmp(query, "select", 6) == 0) - return true; - - if (wordlen == 6 && pg_strncasecmp(query, "values", 6) == 0) - return true; - - return false; -} - - /* * Test if the current user is a database superuser. */ diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl index 9f0b6cf8ca..b5fedbc091 100644 --- a/src/bin/psql/t/001_basic.pl +++ b/src/bin/psql/t/001_basic.pl @@ -161,7 +161,7 @@ psql_like( '\errverbose with no previous error'); # There are three main ways to run a query that might affect -# \errverbose: The normal way, using a cursor by setting FETCH_COUNT, +# \errverbose: The normal way, piecemeal retrieval using FETCH_COUNT, # and using \gdesc. Test them all. like( @@ -184,10 +184,10 @@ like( "\\set FETCH_COUNT 1\nSELECT error;\n\\errverbose", on_error_stop => 0))[2], qr/\A^psql:<stdin>:2: ERROR: .*$ -^LINE 2: SELECT error;$ +^LINE 1: SELECT error;$ ^ *^.*$ ^psql:<stdin>:3: error: ERROR: [0-9A-Z]{5}: .*$ -^LINE 2: SELECT error;$ +^LINE 1: SELECT error;$ ^ *^.*$ ^LOCATION: .*$/m, '\errverbose after FETCH_COUNT query with error'); diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 69060fe3c0..8580db7c00 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -4755,7 +4755,7 @@ number of rows: 0 last error message: syntax error at end of input \echo 'last error code:' :LAST_ERROR_SQLSTATE last error code: 42601 --- check row count for a cursor-fetched query +-- check row count for a query with chunked results \set FETCH_COUNT 10 select unique2 from tenk1 order by unique2 limit 19; unique2 @@ -4787,7 +4787,7 @@ error: false error code: 00000 \echo 'number of rows:' :ROW_COUNT number of rows: 19 --- cursor-fetched query with an error after the first group +-- chunked results with an error after the first chunk select 1/(15-unique2) from tenk1 order by unique2 limit 19; ?column? ---------- @@ -4801,6 +4801,11 @@ select 1/(15-unique2) from tenk1 order by unique2 limit 19; 0 0 0 + 0 + 0 + 0 + 0 + 1 ERROR: division by zero \echo 'error:' :ERROR error: true diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql index 129f853353..33076cad79 100644 --- a/src/test/regress/sql/psql.sql +++ b/src/test/regress/sql/psql.sql @@ -1161,14 +1161,14 @@ SELECT 4 AS \gdesc \echo 'last error message:' :LAST_ERROR_MESSAGE \echo 'last error code:' :LAST_ERROR_SQLSTATE --- check row count for a cursor-fetched query +-- check row count for a query with chunked results \set FETCH_COUNT 10 select unique2 from tenk1 order by unique2 limit 19; \echo 'error:' :ERROR \echo 'error code:' :SQLSTATE \echo 'number of rows:' :ROW_COUNT --- cursor-fetched query with an error after the first group +-- chunked results with an error after the first chunk select 1/(15-unique2) from tenk1 order by unique2 limit 19; \echo 'error:' :ERROR \echo 'error code:' :SQLSTATE -- 2.34.1 Attachments: [text/plain] v7-0001-Implement-retrieval-of-results-in-chunks-with-lib.patch (18.7K, ../../[email protected]/2-v7-0001-Implement-retrieval-of-results-in-chunks-with-lib.patch) download | inline diff: From 0fefaee7d5b3003ad0d089ea9e92675c6f50245f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20V=C3=A9rit=C3=A9?= <[email protected]> Date: Mon, 1 Apr 2024 19:46:20 +0200 Subject: [PATCH v7 1/2] Implement retrieval of results in chunks with libpq. This mode is similar to the single-row mode except that chunks of results contain up to N rows instead of a single row. It is meant to reduce the overhead of the row-by-row allocations for large result sets. The mode is selected with PQsetChunkedRowsMode(int maxRows) and results have the new status code PGRES_TUPLES_CHUNK. --- doc/src/sgml/libpq.sgml | 98 +++++++++++---- .../libpqwalreceiver/libpqwalreceiver.c | 1 + src/bin/pg_amcheck/pg_amcheck.c | 1 + src/interfaces/libpq/exports.txt | 1 + src/interfaces/libpq/fe-exec.c | 117 +++++++++++++++--- src/interfaces/libpq/libpq-fe.h | 4 +- src/interfaces/libpq/libpq-int.h | 7 +- 7 files changed, 185 insertions(+), 44 deletions(-) diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index d3e87056f2..1814921d5a 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -3545,7 +3545,20 @@ ExecStatusType PQresultStatus(const PGresult *res); The <structname>PGresult</structname> contains a single result tuple from the current command. This status occurs only when single-row mode has been selected for the query - (see <xref linkend="libpq-single-row-mode"/>). + (see <xref linkend="libpq-chunked-results-modes"/>). + </para> + </listitem> + </varlistentry> + + <varlistentry id="libpq-pgres-tuples-chunk"> + <term><literal>PGRES_TUPLES_CHUNK</literal></term> + <listitem> + <para> + The <structname>PGresult</structname> contains several tuples + from the current command. The count of tuples cannot exceed + the maximum passed to <xref linkend="libpq-PQsetChunkedRowsMode"/>. + This status occurs only when the chunked mode has been selected + for the query (see <xref linkend="libpq-chunked-results-modes"/>). </para> </listitem> </varlistentry> @@ -5197,8 +5210,8 @@ PGresult *PQgetResult(PGconn *conn); <para> Another frequently-desired feature that can be obtained with <xref linkend="libpq-PQsendQuery"/> and <xref linkend="libpq-PQgetResult"/> - is retrieving large query results a row at a time. This is discussed - in <xref linkend="libpq-single-row-mode"/>. + is retrieving large query results a limited number of rows at a time. This is discussed + in <xref linkend="libpq-chunked-results-modes"/>. </para> <para> @@ -5562,12 +5575,13 @@ int PQflush(PGconn *conn); </para> <para> - To enter single-row mode, call <function>PQsetSingleRowMode</function> - before retrieving results with <function>PQgetResult</function>. - This mode selection is effective only for the query currently - being processed. For more information on the use of - <function>PQsetSingleRowMode</function>, - refer to <xref linkend="libpq-single-row-mode"/>. + To enter single-row or chunked modes, call + respectively <function>PQsetSingleRowMode</function> + or <function>PQsetChunkedRowsMode</function> before retrieving results + with <function>PQgetResult</function>. This mode selection is effective + only for the query currently being processed. For more information on the + use of these functions refer + to <xref linkend="libpq-chunked-results-modes" />. </para> <para> @@ -5934,10 +5948,10 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; </sect2> </sect1> - <sect1 id="libpq-single-row-mode"> - <title>Retrieving Query Results Row-by-Row</title> + <sect1 id="libpq-chunked-results-modes"> + <title>Retrieving Query Results in chunks</title> - <indexterm zone="libpq-single-row-mode"> + <indexterm zone="libpq-chunked-results-modes"> <primary>libpq</primary> <secondary>single-row mode</secondary> </indexterm> @@ -5948,13 +5962,15 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; <structname>PGresult</structname>. This can be unworkable for commands that return a large number of rows. For such cases, applications can use <xref linkend="libpq-PQsendQuery"/> and <xref linkend="libpq-PQgetResult"/> in - <firstterm>single-row mode</firstterm>. In this mode, the result row(s) are - returned to the application one at a time, as they are received from the - server. + <firstterm>single-row mode</firstterm> or <firstterm>chunked mode</firstterm>. + In these modes, the result row(s) are returned to the application one at a + time for the single-row mode and by chunks for the chunked mode, as they + are received from the server. </para> <para> - To enter single-row mode, call <xref linkend="libpq-PQsetSingleRowMode"/> + To enter these modes, call <xref linkend="libpq-PQsetSingleRowMode"/> + or <xref linkend="libpq-PQsetChunkedRowsMode"/> immediately after a successful call of <xref linkend="libpq-PQsendQuery"/> (or a sibling function). This mode selection is effective only for the currently executing query. Then call <xref linkend="libpq-PQgetResult"/> @@ -5962,7 +5978,8 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; linkend="libpq-async"/>. If the query returns any rows, they are returned as individual <structname>PGresult</structname> objects, which look like normal query results except for having status code - <literal>PGRES_SINGLE_TUPLE</literal> instead of + <literal>PGRES_SINGLE_TUPLE</literal> for the single-row mode and + <literal>PGRES_TUPLES_CHUNK</literal> for the chunked mode, instead of <literal>PGRES_TUPLES_OK</literal>. After the last row, or immediately if the query returns zero rows, a zero-row object with status <literal>PGRES_TUPLES_OK</literal> is returned; this is the signal that no @@ -5975,9 +5992,9 @@ UPDATE mytable SET x = x + 1 WHERE id = 42; </para> <para> - When using pipeline mode, single-row mode needs to be activated for each - query in the pipeline before retrieving results for that query - with <function>PQgetResult</function>. + When using pipeline mode, the single-row or chunked mode need to be + activated for each query in the pipeline before retrieving results for that + query with <function>PQgetResult</function>. See <xref linkend="libpq-pipeline-mode"/> for more information. </para> @@ -6011,14 +6028,49 @@ int PQsetSingleRowMode(PGconn *conn); </variablelist> </para> + <para> + <variablelist> + <varlistentry id="libpq-PQsetChunkedRowsMode"> + <term><function>PQsetChunkedRowsMode</function> + <indexterm><primary>PQsetChunkedRowsMode</primary></indexterm></term> + <listitem> + <para> + Select to receive the results for the currently-executing query in chunks. + +<synopsis> + int PQsetChunkedRowsMode(PGconn *conn, + int maxRows); +</synopsis> + </para> + + <para> + This function is similar to <xref linkend="libpq-PQsetSingleRowMode"/>, + except that it can retrieve <replaceable>maxRows</replaceable> rows + per call to <xref linkend="libpq-PQgetResult"/> instead of a single row. + This function can only be called immediately after + <xref linkend="libpq-PQsendQuery"/> or one of its sibling functions, + before any other operation on the connection such as + <xref linkend="libpq-PQconsumeInput"/> or + <xref linkend="libpq-PQgetResult"/>. If called at the correct time, + the function activates the chunked mode for the current query and + returns 1. Otherwise the mode stays unchanged and the function + returns 0. In any case, the mode reverts to normal after + completion of the current query. + </para> + </listitem> + </varlistentry> + </variablelist> + </para> + <caution> <para> While processing a query, the server may return some rows and then encounter an error, causing the query to be aborted. Ordinarily, <application>libpq</application> discards any such rows and reports only the - error. But in single-row mode, those rows will have already been - returned to the application. Hence, the application will see some - <literal>PGRES_SINGLE_TUPLE</literal> <structname>PGresult</structname> + error. But in the single-row or chunked modes, those rows will have already + been returned to the application. Hence, the application will see some + <literal>PGRES_SINGLE_TUPLE</literal> or <literal>PGRES_TUPLES_CHUNK</literal> + <structname>PGresult</structname> objects followed by a <literal>PGRES_FATAL_ERROR</literal> object. For proper transactional behavior, the application must be designed to discard or undo whatever has been done with the previously-processed diff --git a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c index 761bf0f677..83a465a390 100644 --- a/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c +++ b/src/backend/replication/libpqwalreceiver/libpqwalreceiver.c @@ -1249,6 +1249,7 @@ libpqrcv_exec(WalReceiverConn *conn, const char *query, switch (PQresultStatus(pgres)) { case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: case PGRES_TUPLES_OK: walres->status = WALRCV_OK_TUPLES; libpqrcv_processTuples(pgres, walres, nRetTypes, retTypes); diff --git a/src/bin/pg_amcheck/pg_amcheck.c b/src/bin/pg_amcheck/pg_amcheck.c index e5f9eedc47..728305a7cf 100644 --- a/src/bin/pg_amcheck/pg_amcheck.c +++ b/src/bin/pg_amcheck/pg_amcheck.c @@ -989,6 +989,7 @@ should_processing_continue(PGresult *res) case PGRES_COPY_IN: case PGRES_COPY_BOTH: case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: case PGRES_PIPELINE_SYNC: case PGRES_PIPELINE_ABORTED: return false; diff --git a/src/interfaces/libpq/exports.txt b/src/interfaces/libpq/exports.txt index 9fbd3d3407..c7d01958ab 100644 --- a/src/interfaces/libpq/exports.txt +++ b/src/interfaces/libpq/exports.txt @@ -202,3 +202,4 @@ PQcancelSocket 199 PQcancelErrorMessage 200 PQcancelReset 201 PQcancelFinish 202 +PQsetChunkedRowsMode 203 diff --git a/src/interfaces/libpq/fe-exec.c b/src/interfaces/libpq/fe-exec.c index c02a9180b2..b9a73b583e 100644 --- a/src/interfaces/libpq/fe-exec.c +++ b/src/interfaces/libpq/fe-exec.c @@ -41,7 +41,8 @@ char *const pgresStatus[] = { "PGRES_COPY_BOTH", "PGRES_SINGLE_TUPLE", "PGRES_PIPELINE_SYNC", - "PGRES_PIPELINE_ABORTED" + "PGRES_PIPELINE_ABORTED", + "PGRES_TUPLES_CHUNK" }; /* We return this if we're unable to make a PGresult at all */ @@ -83,7 +84,7 @@ static int check_field_number(const PGresult *res, int field_num); static void pqPipelineProcessQueue(PGconn *conn); static int pqPipelineSyncInternal(PGconn *conn, bool immediate_flush); static int pqPipelineFlush(PGconn *conn); - +static bool canChangeRowMode(PGconn *conn); /* ---------------- * Space management for PGresult. @@ -200,6 +201,7 @@ PQmakeEmptyPGresult(PGconn *conn, ExecStatusType status) case PGRES_COPY_IN: case PGRES_COPY_BOTH: case PGRES_SINGLE_TUPLE: + case PGRES_TUPLES_CHUNK: /* non-error cases */ break; default: @@ -913,8 +915,9 @@ pqPrepareAsyncResult(PGconn *conn) /* * Replace conn->result with next_result, if any. In the normal case * there isn't a next result and we're just dropping ownership of the - * current result. In single-row mode this restores the situation to what - * it was before we created the current single-row result. + * current result. In single-row and chunked modes this restores the + * situation to what it was before we created the current single-row or + * chunk-of-rows result. */ conn->result = conn->next_result; conn->error_result = false; /* next_result is never an error */ @@ -1200,10 +1203,11 @@ pqSaveParameterStatus(PGconn *conn, const char *name, const char *value) * (Such a string should already be translated via libpq_gettext().) * If it is left NULL, the error is presumed to be "out of memory". * - * In single-row mode, we create a new result holding just the current row, - * stashing the previous result in conn->next_result so that it becomes - * active again after pqPrepareAsyncResult(). This allows the result metadata - * (column descriptions) to be carried forward to each result row. + * In single-row or chunked mode, we create a new result holding just the + * current set of rows, stashing the previous result in conn->next_result so + * that it becomes active again after pqPrepareAsyncResult(). This allows the + * result metadata (column descriptions) to be carried forward to each result + * row. */ int pqRowProcessor(PGconn *conn, const char **errmsgp) @@ -1228,6 +1232,28 @@ pqRowProcessor(PGconn *conn, const char **errmsgp) if (!res) return 0; } + else if (conn->rowsChunkSize > 0) + { + /* + * In chunked mode, make a new PGresult that will hold N rows; the + * original conn->result is left unchanged, as in the single-row mode. + */ + if (!conn->chunk_result) + { + /* Allocate and initialize the result to hold a chunk of rows */ + res = PQcopyResult(res, + PG_COPYRES_ATTRS | PG_COPYRES_EVENTS | + PG_COPYRES_NOTICEHOOKS); + if (!res) + return 0; + /* Change result status to special chunk-of-rows value */ + res->resultStatus = PGRES_TUPLES_CHUNK; + /* Keep this result to reuse for the next rows of the chunk */ + conn->chunk_result = res; + } + else + res = conn->chunk_result; /* Use the current chunk */ + } /* * Basically we just allocate space in the PGresult for each field and @@ -1290,6 +1316,21 @@ pqRowProcessor(PGconn *conn, const char **errmsgp) conn->asyncStatus = PGASYNC_READY_MORE; } + /* + * In chunked mode, if the count has reached the requested limit, make the + * rows of the current chunk available immediately. + */ + else if (conn->rowsChunkSize > 0 && res->ntups >= conn->rowsChunkSize) + { + /* Stash old result for re-use later */ + conn->next_result = conn->result; + conn->result = res; + /* Do not reuse that chunk of results */ + conn->chunk_result = NULL; + /* And mark the result ready to return */ + conn->asyncStatus = PGASYNC_READY_MORE; + } + return 1; fail: @@ -1745,8 +1786,9 @@ PQsendQueryStart(PGconn *conn, bool newQuery) */ pqClearAsyncResult(conn); - /* reset single-row processing mode */ + /* reset row-by-row and chunked processing modes */ conn->singleRowMode = false; + conn->rowsChunkSize = 0; } /* ready to send command message */ @@ -1930,25 +1972,51 @@ sendFailed: */ int PQsetSingleRowMode(PGconn *conn) +{ + if (canChangeRowMode(conn)) + { + conn->singleRowMode = true; + return 1; + } + else + return 0; +} + +/* + * Select chunked results processing mode + */ +int +PQsetChunkedRowsMode(PGconn *conn, int chunkSize) +{ + if (chunkSize >= 0 && canChangeRowMode(conn)) + { + conn->rowsChunkSize = chunkSize; + return 1; + } + else + return 0; +} + +static +bool +canChangeRowMode(PGconn *conn) { /* - * Only allow setting the flag when we have launched a query and not yet - * received any results. + * Only allow setting the row-by-row or by-chunks modes when we have + * launched a query and not yet received any results. */ if (!conn) - return 0; + return false; if (conn->asyncStatus != PGASYNC_BUSY) - return 0; + return false; if (!conn->cmd_queue_head || (conn->cmd_queue_head->queryclass != PGQUERY_SIMPLE && conn->cmd_queue_head->queryclass != PGQUERY_EXTENDED)) - return 0; + return false; if (pgHavePendingResult(conn)) - return 0; + return false; - /* OK, set flag */ - conn->singleRowMode = true; - return 1; + return true; } /* @@ -2115,6 +2183,16 @@ PQgetResult(PGconn *conn) break; case PGASYNC_READY: + /* + * If there is a pending chunk of results, return it + */ + if (conn->chunk_result != NULL) + { + res = conn->chunk_result; + conn->chunk_result = NULL; + break; + } + res = pqPrepareAsyncResult(conn); /* Advance the queue as appropriate */ @@ -3173,10 +3251,11 @@ pqPipelineProcessQueue(PGconn *conn) } /* - * Reset single-row processing mode. (Client has to set it up for each + * Reset to full result sets mode. (Client has to set it up for each * query, if desired.) */ conn->singleRowMode = false; + conn->rowsChunkSize = 0; /* * If there are no further commands to process in the queue, get us in diff --git a/src/interfaces/libpq/libpq-fe.h b/src/interfaces/libpq/libpq-fe.h index 09b485bd2b..0cea4f6b5b 100644 --- a/src/interfaces/libpq/libpq-fe.h +++ b/src/interfaces/libpq/libpq-fe.h @@ -112,8 +112,9 @@ typedef enum PGRES_COPY_BOTH, /* Copy In/Out data transfer in progress */ PGRES_SINGLE_TUPLE, /* single tuple from larger resultset */ PGRES_PIPELINE_SYNC, /* pipeline synchronization point */ - PGRES_PIPELINE_ABORTED /* Command didn't run because of an abort + PGRES_PIPELINE_ABORTED, /* Command didn't run because of an abort * earlier in a pipeline */ + PGRES_TUPLES_CHUNK /* set of tuples from larger resultset */ } ExecStatusType; typedef enum @@ -489,6 +490,7 @@ extern int PQsendQueryPrepared(PGconn *conn, const int *paramFormats, int resultFormat); extern int PQsetSingleRowMode(PGconn *conn); +extern int PQsetChunkedRowsMode(PGconn *conn, int chunkSize); extern PGresult *PQgetResult(PGconn *conn); /* Routines for managing an asynchronous query */ diff --git a/src/interfaces/libpq/libpq-int.h b/src/interfaces/libpq/libpq-int.h index 9c05f11a6e..23c7a399ab 100644 --- a/src/interfaces/libpq/libpq-int.h +++ b/src/interfaces/libpq/libpq-int.h @@ -435,6 +435,8 @@ struct pg_conn * sending semantics */ PGpipelineStatus pipelineStatus; /* status of pipeline mode */ bool singleRowMode; /* return current query result row-by-row? */ + int rowsChunkSize; /* non-zero to return query results by chunks + * not exceeding that number of rows */ char copy_is_binary; /* 1 = copy binary, 0 = copy text */ int copy_already_done; /* # bytes already returned in COPY OUT */ PGnotify *notifyHead; /* oldest unreported Notify msg */ @@ -540,7 +542,10 @@ struct pg_conn */ PGresult *result; /* result being constructed */ bool error_result; /* do we need to make an ERROR result? */ - PGresult *next_result; /* next result (used in single-row mode) */ + PGresult *next_result; /* next result (used in single-row and + * by-chunks modes) */ + PGresult *chunk_result; /* current chunk of results (limited to + * rowsChunkSize) */ /* Assorted state for SASL, SSL, GSS, etc */ const pg_fe_sasl_mech *sasl; -- 2.34.1 [text/plain] v7-0002-Reimplement-FETCH_COUNT-with-the-chunked-mode-in-.patch (20.4K, ../../[email protected]/3-v7-0002-Reimplement-FETCH_COUNT-with-the-chunked-mode-in-.patch) download | inline diff: From 33f043aeaf3969e66f6c80af6ef6ea27499b4740 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Daniel=20V=C3=A9rit=C3=A9?= <[email protected]> Date: Mon, 1 Apr 2024 19:46:42 +0200 Subject: [PATCH v7 2/2] Reimplement FETCH_COUNT with the chunked mode in libpq. Cursors were used only when the command starts with the keyword "SELECT", excluding queries that start with "WITH" or "UPDATE" or "INSERT" that may also return large result sets. --- src/bin/psql/common.c | 538 ++++++++++------------------- src/bin/psql/t/001_basic.pl | 6 +- src/test/regress/expected/psql.out | 9 +- src/test/regress/sql/psql.sql | 4 +- 4 files changed, 189 insertions(+), 368 deletions(-) diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c index 2830bde495..2112e1a423 100644 --- a/src/bin/psql/common.c +++ b/src/bin/psql/common.c @@ -31,7 +31,6 @@ #include "settings.h" static bool DescribeQuery(const char *query, double *elapsed_msec); -static bool ExecQueryUsingCursor(const char *query, double *elapsed_msec); static int ExecQueryAndProcessResults(const char *query, double *elapsed_msec, bool *svpt_gone_p, @@ -40,8 +39,6 @@ static int ExecQueryAndProcessResults(const char *query, const printQueryOpt *opt, FILE *printQueryFout); static bool command_no_begin(const char *query); -static bool is_select_command(const char *query); - /* * openQueryOutputFile --- attempt to open a query output file @@ -373,6 +370,7 @@ AcceptResult(const PGresult *result, bool show_error) { case PGRES_COMMAND_OK: case PGRES_TUPLES_OK: + case PGRES_TUPLES_CHUNK: case PGRES_EMPTY_QUERY: case PGRES_COPY_IN: case PGRES_COPY_OUT: @@ -1135,16 +1133,10 @@ SendQuery(const char *query) /* Describe query's result columns, without executing it */ OK = DescribeQuery(query, &elapsed_msec); } - else if (pset.fetch_count <= 0 || pset.gexec_flag || - pset.crosstab_flag || !is_select_command(query)) - { - /* Default fetch-it-all-and-print mode */ - OK = (ExecQueryAndProcessResults(query, &elapsed_msec, &svpt_gone, false, 0, NULL, NULL) > 0); - } else { - /* Fetch-in-segments mode */ - OK = ExecQueryUsingCursor(query, &elapsed_msec); + /* Default fetch-and-print mode */ + OK = (ExecQueryAndProcessResults(query, &elapsed_msec, &svpt_gone, false, 0, NULL, NULL) > 0); } if (!OK && pset.echo == PSQL_ECHO_ERRORS) @@ -1396,6 +1388,47 @@ DescribeQuery(const char *query, double *elapsed_msec) return OK; } +/* + * Check if an output stream for \g needs to be opened, and if + * yes, open it. + * Return false if an error occurred, true otherwise. + */ +static bool +SetupGOutput(PGresult *result, FILE **gfile_fout, bool *is_pipe) +{ + ExecStatusType status = PQresultStatus(result); + if (pset.gfname != NULL && /* there is a \g file or program */ + *gfile_fout == NULL && /* and it's not already opened */ + (status == PGRES_TUPLES_OK || + status == PGRES_TUPLES_CHUNK || + status == PGRES_COPY_OUT)) + { + if (openQueryOutputFile(pset.gfname, gfile_fout, is_pipe)) + { + if (is_pipe) + disable_sigpipe_trap(); + } + else + return false; + } + return true; +} + +static void +CloseGOutput(FILE *gfile_fout, bool is_pipe) +{ + /* close \g file if we opened it */ + if (gfile_fout) + { + if (is_pipe) + { + SetShellResultVariables(pclose(gfile_fout)); + restore_sigpipe_trap(); + } + else + fclose(gfile_fout); + } +} /* * ExecQueryAndProcessResults: utility function for use by SendQuery() @@ -1429,9 +1462,14 @@ ExecQueryAndProcessResults(const char *query, instr_time before, after; PGresult *result; + FILE *gfile_fout = NULL; bool gfile_is_pipe = false; + int64 total_tuples = 0; + int flush_error = 0; + bool is_pager = false; + if (timing) INSTR_TIME_SET_CURRENT(before); else @@ -1454,6 +1492,23 @@ ExecQueryAndProcessResults(const char *query, return -1; } + /* + * Fetch the result in chunks if FETCH_COUNT is set. + * We don't enable chunking if SHOW_ALL_RESULTS is false, since that + * requires us to accumulate all rows before we can tell what should be + * displayed, which would counter the idea of FETCH_COUNT. + * Chunk fetching is also disabled if \gset, \crosstab, \gexec and \watch + * are used. + */ + if (pset.fetch_count > 0 && !pset.crosstab_flag && !pset.gexec_flag && !is_watch + && !pset.gset_prefix && pset.show_all_results) + { + if (!PQsetChunkedRowsMode(pset.db, pset.fetch_count)) + { + pg_log_warning("fetching results in chunks mode is unavailable"); + } + } + /* * If SIGINT is sent while the query is processing, the interrupt will be * consumed. The user's intention, though, is to cancel the entire watch @@ -1477,6 +1532,8 @@ ExecQueryAndProcessResults(const char *query, ExecStatusType result_status; PGresult *next_result; bool last; + /* whether the output starts before results are fully fetched */ + bool partial_display = false; if (!AcceptResult(result, false)) { @@ -1572,20 +1629,9 @@ ExecQueryAndProcessResults(const char *query, } else if (pset.gfname) { - /* send to \g file, which we may have opened already */ - if (gfile_fout == NULL) - { - if (openQueryOutputFile(pset.gfname, - &gfile_fout, &gfile_is_pipe)) - { - if (gfile_is_pipe) - disable_sigpipe_trap(); - copy_stream = gfile_fout; - } - else - success = false; - } - else + /* COPY followed by \g filename or \g |program */ + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (success) copy_stream = gfile_fout; } else @@ -1603,6 +1649,90 @@ ExecQueryAndProcessResults(const char *query, success &= HandleCopyResult(&result, copy_stream); } + if (result_status == PGRES_TUPLES_CHUNK) + { + FILE *tuples_fout = printQueryFout ? printQueryFout : stdout; + printQueryOpt my_popt = pset.popt; + + total_tuples = 0; + partial_display = true; + + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (gfile_fout) + tuples_fout = gfile_fout; + + /* initialize print options for partial table output */ + my_popt.topt.start_table = true; + my_popt.topt.stop_table = false; + my_popt.topt.prior_records = 0; + + while (success) + { + /* pager: open at most once per resultset */ + if (tuples_fout == stdout && !is_pager) + { + tuples_fout = PageOutput(INT_MAX, &(my_popt.topt)); + is_pager = true; + } + /* display the current chunk of results unless the output stream is not working */ + if (!flush_error) + { + printQuery(result, &my_popt, tuples_fout, is_pager, pset.logfile); + flush_error = fflush(tuples_fout); + } + + /* after the first result set, disallow header decoration */ + my_popt.topt.start_table = false; + my_popt.topt.prior_records += PQntuples(result); + total_tuples += PQntuples(result); + + ClearOrSaveResult(result); + + result = PQgetResult(pset.db); + if (result == NULL) + { + /* + * Error. We expect a PGRES_TUPLES_OK result with + * zero tuple in it to finish the fetch sequence. + */ + success = false; + if (is_pager) + ClosePager(tuples_fout); + break; + } + else if (PQresultStatus(result) == PGRES_TUPLES_OK) + { + /* + * The last row has been read. Display the footer. + */ + my_popt.topt.stop_table = true; + printQuery(result, &my_popt, tuples_fout, is_pager, pset.logfile); + total_tuples += PQntuples(result); + + if (is_pager) + ClosePager(tuples_fout); + ClearOrSaveResult(result); + result = NULL; + break; + } + else if (PQresultStatus(result) != PGRES_TUPLES_CHUNK) + { + /* + * Error. We expect either PGRES_TUPLES_CHUNK or + * PGRES_TUPLES_OK. + */ + if (is_pager) + ClosePager(tuples_fout); + success = false; + AcceptResult(result, true); /* display error whenever appropriate */ + SetResultVariables(result, success); + break; + } + } + } + else + partial_display = false; + /* * Check PQgetResult() again. In the typical case of a single-command * string, it will return NULL. Otherwise, we'll have other results @@ -1631,7 +1761,7 @@ ExecQueryAndProcessResults(const char *query, } /* this may or may not print something depending on settings */ - if (result != NULL) + if (result != NULL && !partial_display) { /* * If results need to be printed into the file specified by \g, @@ -1640,32 +1770,33 @@ ExecQueryAndProcessResults(const char *query, * tuple output, but it's still used for status output. */ FILE *tuples_fout = printQueryFout; - bool do_print = true; - - if (PQresultStatus(result) == PGRES_TUPLES_OK && - pset.gfname) - { - if (gfile_fout == NULL) - { - if (openQueryOutputFile(pset.gfname, - &gfile_fout, &gfile_is_pipe)) - { - if (gfile_is_pipe) - disable_sigpipe_trap(); - } - else - success = do_print = false; - } + success = SetupGOutput(result, &gfile_fout, &gfile_is_pipe); + if (gfile_fout) tuples_fout = gfile_fout; - } - if (do_print) + if (success) success &= PrintQueryResult(result, last, opt, tuples_fout, printQueryFout); } /* set variables from last result */ if (!is_watch && last) - SetResultVariables(result, success); + { + if (!partial_display) + SetResultVariables(result, success); + else if (success) + { + /* + * fake SetResultVariables(). If an error occurred when + * retrieving chunks, these variables have been set already. + */ + char buf[32]; + + SetVariable(pset.vars, "ERROR", "false"); + SetVariable(pset.vars, "SQLSTATE", "00000"); + snprintf(buf, sizeof(buf), INT64_FORMAT, total_tuples); + SetVariable(pset.vars, "ROW_COUNT", buf); + } + } ClearOrSaveResult(result); result = next_result; @@ -1677,17 +1808,7 @@ ExecQueryAndProcessResults(const char *query, } } - /* close \g file if we opened it */ - if (gfile_fout) - { - if (gfile_is_pipe) - { - SetShellResultVariables(pclose(gfile_fout)); - restore_sigpipe_trap(); - } - else - fclose(gfile_fout); - } + CloseGOutput(gfile_fout, gfile_is_pipe); /* may need this to recover from conn loss during COPY */ if (!CheckConnection()) @@ -1700,274 +1821,6 @@ ExecQueryAndProcessResults(const char *query, } -/* - * ExecQueryUsingCursor: run a SELECT-like query using a cursor - * - * This feature allows result sets larger than RAM to be dealt with. - * - * Returns true if the query executed successfully, false otherwise. - * - * If pset.timing is on, total query time (exclusive of result-printing) is - * stored into *elapsed_msec. - */ -static bool -ExecQueryUsingCursor(const char *query, double *elapsed_msec) -{ - bool OK = true; - PGresult *result; - PQExpBufferData buf; - printQueryOpt my_popt = pset.popt; - bool timing = pset.timing; - FILE *fout; - bool is_pipe; - bool is_pager = false; - bool started_txn = false; - int64 total_tuples = 0; - int ntuples; - int fetch_count; - char fetch_cmd[64]; - instr_time before, - after; - int flush_error; - - *elapsed_msec = 0; - - /* initialize print options for partial table output */ - my_popt.topt.start_table = true; - my_popt.topt.stop_table = false; - my_popt.topt.prior_records = 0; - - if (timing) - INSTR_TIME_SET_CURRENT(before); - else - INSTR_TIME_SET_ZERO(before); - - /* if we're not in a transaction, start one */ - if (PQtransactionStatus(pset.db) == PQTRANS_IDLE) - { - result = PQexec(pset.db, "BEGIN"); - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - if (!OK) - return false; - started_txn = true; - } - - /* Send DECLARE CURSOR */ - initPQExpBuffer(&buf); - appendPQExpBuffer(&buf, "DECLARE _psql_cursor NO SCROLL CURSOR FOR\n%s", - query); - - result = PQexec(pset.db, buf.data); - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - if (!OK) - SetResultVariables(result, OK); - ClearOrSaveResult(result); - termPQExpBuffer(&buf); - if (!OK) - goto cleanup; - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - /* - * In \gset mode, we force the fetch count to be 2, so that we will throw - * the appropriate error if the query returns more than one row. - */ - if (pset.gset_prefix) - fetch_count = 2; - else - fetch_count = pset.fetch_count; - - snprintf(fetch_cmd, sizeof(fetch_cmd), - "FETCH FORWARD %d FROM _psql_cursor", - fetch_count); - - /* prepare to write output to \g argument, if any */ - if (pset.gfname) - { - if (!openQueryOutputFile(pset.gfname, &fout, &is_pipe)) - { - OK = false; - goto cleanup; - } - if (is_pipe) - disable_sigpipe_trap(); - } - else - { - fout = pset.queryFout; - is_pipe = false; /* doesn't matter */ - } - - /* clear any pre-existing error indication on the output stream */ - clearerr(fout); - - for (;;) - { - if (timing) - INSTR_TIME_SET_CURRENT(before); - - /* get fetch_count tuples at a time */ - result = PQexec(pset.db, fetch_cmd); - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - if (PQresultStatus(result) != PGRES_TUPLES_OK) - { - /* shut down pager before printing error message */ - if (is_pager) - { - ClosePager(fout); - is_pager = false; - } - - OK = AcceptResult(result, true); - Assert(!OK); - SetResultVariables(result, OK); - ClearOrSaveResult(result); - break; - } - - if (pset.gset_prefix) - { - /* StoreQueryTuple will complain if not exactly one row */ - OK = StoreQueryTuple(result); - ClearOrSaveResult(result); - break; - } - - /* - * Note we do not deal with \gdesc, \gexec or \crosstabview modes here - */ - - ntuples = PQntuples(result); - total_tuples += ntuples; - - if (ntuples < fetch_count) - { - /* this is the last result set, so allow footer decoration */ - my_popt.topt.stop_table = true; - } - else if (fout == stdout && !is_pager) - { - /* - * If query requires multiple result sets, hack to ensure that - * only one pager instance is used for the whole mess - */ - fout = PageOutput(INT_MAX, &(my_popt.topt)); - is_pager = true; - } - - printQuery(result, &my_popt, fout, is_pager, pset.logfile); - - ClearOrSaveResult(result); - - /* after the first result set, disallow header decoration */ - my_popt.topt.start_table = false; - my_popt.topt.prior_records += ntuples; - - /* - * Make sure to flush the output stream, so intermediate results are - * visible to the client immediately. We check the results because if - * the pager dies/exits/etc, there's no sense throwing more data at - * it. - */ - flush_error = fflush(fout); - - /* - * Check if we are at the end, if a cancel was pressed, or if there - * were any errors either trying to flush out the results, or more - * generally on the output stream at all. If we hit any errors - * writing things to the stream, we presume $PAGER has disappeared and - * stop bothering to pull down more data. - */ - if (ntuples < fetch_count || cancel_pressed || flush_error || - ferror(fout)) - break; - } - - if (pset.gfname) - { - /* close \g argument file/pipe */ - if (is_pipe) - { - SetShellResultVariables(pclose(fout)); - restore_sigpipe_trap(); - } - else - fclose(fout); - } - else if (is_pager) - { - /* close transient pager */ - ClosePager(fout); - } - - if (OK) - { - /* - * We don't have a PGresult here, and even if we did it wouldn't have - * the right row count, so fake SetResultVariables(). In error cases, - * we already set the result variables above. - */ - char buf[32]; - - SetVariable(pset.vars, "ERROR", "false"); - SetVariable(pset.vars, "SQLSTATE", "00000"); - snprintf(buf, sizeof(buf), INT64_FORMAT, total_tuples); - SetVariable(pset.vars, "ROW_COUNT", buf); - } - -cleanup: - if (timing) - INSTR_TIME_SET_CURRENT(before); - - /* - * We try to close the cursor on either success or failure, but on failure - * ignore the result (it's probably just a bleat about being in an aborted - * transaction) - */ - result = PQexec(pset.db, "CLOSE _psql_cursor"); - if (OK) - { - OK = AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - } - else - PQclear(result); - - if (started_txn) - { - result = PQexec(pset.db, OK ? "COMMIT" : "ROLLBACK"); - OK &= AcceptResult(result, true) && - (PQresultStatus(result) == PGRES_COMMAND_OK); - ClearOrSaveResult(result); - } - - if (timing) - { - INSTR_TIME_SET_CURRENT(after); - INSTR_TIME_SUBTRACT(after, before); - *elapsed_msec += INSTR_TIME_GET_MILLISEC(after); - } - - return OK; -} - - /* * Advance the given char pointer over white space and SQL comments. */ @@ -2247,43 +2100,6 @@ command_no_begin(const char *query) } -/* - * Check whether the specified command is a SELECT (or VALUES). - */ -static bool -is_select_command(const char *query) -{ - int wordlen; - - /* - * First advance over any whitespace, comments and left parentheses. - */ - for (;;) - { - query = skip_white_space(query); - if (query[0] == '(') - query++; - else - break; - } - - /* - * Check word length (since "selectx" is not "select"). - */ - wordlen = 0; - while (isalpha((unsigned char) query[wordlen])) - wordlen += PQmblenBounded(&query[wordlen], pset.encoding); - - if (wordlen == 6 && pg_strncasecmp(query, "select", 6) == 0) - return true; - - if (wordlen == 6 && pg_strncasecmp(query, "values", 6) == 0) - return true; - - return false; -} - - /* * Test if the current user is a database superuser. */ diff --git a/src/bin/psql/t/001_basic.pl b/src/bin/psql/t/001_basic.pl index 9f0b6cf8ca..b5fedbc091 100644 --- a/src/bin/psql/t/001_basic.pl +++ b/src/bin/psql/t/001_basic.pl @@ -161,7 +161,7 @@ psql_like( '\errverbose with no previous error'); # There are three main ways to run a query that might affect -# \errverbose: The normal way, using a cursor by setting FETCH_COUNT, +# \errverbose: The normal way, piecemeal retrieval using FETCH_COUNT, # and using \gdesc. Test them all. like( @@ -184,10 +184,10 @@ like( "\\set FETCH_COUNT 1\nSELECT error;\n\\errverbose", on_error_stop => 0))[2], qr/\A^psql:<stdin>:2: ERROR: .*$ -^LINE 2: SELECT error;$ +^LINE 1: SELECT error;$ ^ *^.*$ ^psql:<stdin>:3: error: ERROR: [0-9A-Z]{5}: .*$ -^LINE 2: SELECT error;$ +^LINE 1: SELECT error;$ ^ *^.*$ ^LOCATION: .*$/m, '\errverbose after FETCH_COUNT query with error'); diff --git a/src/test/regress/expected/psql.out b/src/test/regress/expected/psql.out index 69060fe3c0..8580db7c00 100644 --- a/src/test/regress/expected/psql.out +++ b/src/test/regress/expected/psql.out @@ -4755,7 +4755,7 @@ number of rows: 0 last error message: syntax error at end of input \echo 'last error code:' :LAST_ERROR_SQLSTATE last error code: 42601 --- check row count for a cursor-fetched query +-- check row count for a query with chunked results \set FETCH_COUNT 10 select unique2 from tenk1 order by unique2 limit 19; unique2 @@ -4787,7 +4787,7 @@ error: false error code: 00000 \echo 'number of rows:' :ROW_COUNT number of rows: 19 --- cursor-fetched query with an error after the first group +-- chunked results with an error after the first chunk select 1/(15-unique2) from tenk1 order by unique2 limit 19; ?column? ---------- @@ -4801,6 +4801,11 @@ select 1/(15-unique2) from tenk1 order by unique2 limit 19; 0 0 0 + 0 + 0 + 0 + 0 + 1 ERROR: division by zero \echo 'error:' :ERROR error: true diff --git a/src/test/regress/sql/psql.sql b/src/test/regress/sql/psql.sql index 129f853353..33076cad79 100644 --- a/src/test/regress/sql/psql.sql +++ b/src/test/regress/sql/psql.sql @@ -1161,14 +1161,14 @@ SELECT 4 AS \gdesc \echo 'last error message:' :LAST_ERROR_MESSAGE \echo 'last error code:' :LAST_ERROR_SQLSTATE --- check row count for a cursor-fetched query +-- check row count for a query with chunked results \set FETCH_COUNT 10 select unique2 from tenk1 order by unique2 limit 19; \echo 'error:' :ERROR \echo 'error code:' :SQLSTATE \echo 'number of rows:' :ROW_COUNT --- cursor-fetched query with an error after the first group +-- chunked results with an error after the first chunk select 1/(15-unique2) from tenk1 order by unique2 limit 19; \echo 'error:' :ERROR \echo 'error code:' :SQLSTATE -- 2.34.1 ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs @ 2024-04-02 16:50 Tom Lane <[email protected]> parent: Daniel Verite <[email protected]> 0 siblings, 0 replies; 33+ messages in thread From: Tom Lane @ 2024-04-02 16:50 UTC (permalink / raw) To: Daniel Verite <[email protected]>; +Cc: Laurenz Albe <[email protected]>; pgsql-hackers; Robert Haas <[email protected]>; Jakub Wartak <[email protected]> "Daniel Verite" <[email protected]> writes: > Updated patches are attached. I started to look through this, and almost immediately noted - <sect1 id="libpq-single-row-mode"> - <title>Retrieving Query Results Row-by-Row</title> + <sect1 id="libpq-chunked-results-modes"> + <title>Retrieving Query Results in chunks</title> This is a bit problematic, because changing the sect1 ID will change the page's URL, eg https://www.postgresql.org/docs/current/libpq-single-row-mode.html Aside from possibly breaking people's bookmarks, I'm pretty sure this will cause the web docs framework to not recognize any cross-version commonality of the page. How ugly would it be if we left the ID alone? Another idea could be to leave the whole page alone and add a new <sect1> for chunked mode. But ... TBH I'm not convinced that we need the chunked mode at all. We explicitly rejected that idea back when single-row mode was designed, see around here: https://www.postgresql.org/message-id/flat/50173BF7.1070801%40Yahoo.com#7f92ebad0143fb5f575ecb3913c5... and I'm still very skeptical that there's much win to be had. I do not buy that psql's FETCH_COUNT mode is a sufficient reason to add it. FETCH_COUNT mode is not something you'd use non-interactively, and there is enough overhead elsewhere in psql (notably in result-set formatting) that it doesn't seem worth micro-optimizing the part about fetching from libpq. (I see that there was some discussion in that old thread about micro-optimizing single-row mode internally to libpq by making PGresult creation cheaper, which I don't think anyone ever got back to doing. Maybe we should resurrect that.) regards, tom lane ^ permalink raw reply [nested|flat] 33+ messages in thread
* Built-in Raft replication @ 2025-04-14 17:15 Konstantin Osipov <[email protected]> 0 siblings, 3 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-14 17:15 UTC (permalink / raw) To: [email protected] Hi, I am considering starting work on implementing a built-in Raft replication for PostgreSQL. Raft's advantage is that it unifies log replication, cluster configuration/membership/topology management and initial state transfer into a single protocol. Currently the cluster configuration/topology is often managed by Patroni, or similar tools, however, it seems there are certain usability drawbacks with this approach: - it's a separate tool, requiring an external state provider like etcd; raft could store its configuration in system tables; this is also an observability improvement since everyone could look up cluster state the same way as everything else - same for watchdog; raft has a built-in failure detector that's configuration aware; - flexible quorums; currently quorum size is a configurable; with a built-in raft, extending the quorum could be a matter of starting a new node and pointing it to an existing cluster Going forward I can see PostgreSQL providing transparent bouncing on pg_wire level, given that Raft state is now part of the system, so drivers and all cluster nodes could easily see where the leader is. If anyone is working on Raft already I'd be happy to discuss the details. I am fairly new to the PostgreSQL hackers ecosystem so cautious of starting work in isolation/knowing there is no interest in accepting the feature into the trunk. Thanks, -- Konstantin Osipov ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-14 17:44 Kirill Reshke <[email protected]> parent: Konstantin Osipov <[email protected]> 2 siblings, 2 replies; 33+ messages in thread From: Kirill Reshke @ 2025-04-14 17:44 UTC (permalink / raw) To: Konstantin Osipov <[email protected]>; +Cc: [email protected] On Mon, 14 Apr 2025 at 22:15, Konstantin Osipov <[email protected]> wrote: > > Hi, Hi > I am considering starting work on implementing a built-in Raft > replication for PostgreSQL. > Just some thought on top of my mind, if you need my voice here: I have a hard time believing the community will be positive about this change in-core. It has more changes as contrib extension. In fact, if we want a built-in consensus algorithm, Paxos is a better option, because you can use postgresql as local crash-safe storage for single decree paxos, just store your state (ballot number, last voice) in a heap table. OTOH Raft needs to write its own log, and what's worse, it sometimes needs to remove already written parts of it (so, it is not appended only, unlike WAL). If you have a production system which maintains two kinds of logs with different semantics, it is a very hard system to maintain.. There is actually a prod-ready (non open source) implementation of RAFT as extension, called BiHA, by pgpro. Just some thought on top of my mind, if you need my voice here. -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-14 19:00 Konstantin Osipov <[email protected]> parent: Kirill Reshke <[email protected]> 1 sibling, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-14 19:00 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: [email protected] * Kirill Reshke <[email protected]> [25/04/14 20:48]: > > I am considering starting work on implementing a built-in Raft > > replication for PostgreSQL. > > > > Just some thought on top of my mind, if you need my voice here: > > I have a hard time believing the community will be positive about this > change in-core. It has more changes as contrib extension. In fact, if > we want a built-in consensus algorithm, Paxos is a better option, > because you can use postgresql as local crash-safe storage for single > decree paxos, just store your state (ballot number, last voice) in a > heap table. But Raft is a log replication algorithm, not a consensus algorithm. It does use consensus, but that's for leader election. Paxos could be used for log replication, but that would be expensive. In fact etcd uses Raft, and etcd is used by Patroni. So I completely lost your line of thought here. > OTOH Raft needs to write its own log, and what's worse, it sometimes > needs to remove already written parts of it (so, it is not appended > only, unlike WAL). If you have a production system which maintains two > kinds of logs with different semantics, it is a very hard system to > maintain.. My proposal is exactly to replace (or rather, extend) the current synchronous log replication with Raft. Entry removal is possible to stack on top of append-only format, and production implementations exist which do that. So, no, it's a single log, and in fact the current WAL will do. > There is actually a prod-ready (non open source) implementation of > RAFT as extension, called BiHA, by pgpro. My guess biha is an extension since a proprietary code is easier to maintain that way. I'd rather say the fact that there is a proprietary implementation out in the field confirms it could be a good idea to have it in PostgreSQL trunk. In any case I'm interested in contributing to the trunk, not building a proprietary module/fork. -- Konstantin Osipov ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 10:20 Aleksander Alekseev <[email protected]> parent: Konstantin Osipov <[email protected]> 2 siblings, 3 replies; 33+ messages in thread From: Aleksander Alekseev @ 2025-04-15 10:20 UTC (permalink / raw) To: Konstantin Osipov <[email protected]>; +Cc: [email protected] Hi Konstantin, > I am considering starting work on implementing a built-in Raft > replication for PostgreSQL. Generally speaking I like the idea. The more important question IMO is whether we want to maintain Raft within the PostgreSQL core project. Building distributed systems on commodity hardware was a popular idea back in the 2000s. These days you can rent a server with 2 Tb of RAM for something like 2000 USD/month (numbers from my memory that were valid ~5 years ago) which will fit many of the existing businesses (!) in memory. And you can rent another one for a replica, just in order not to recover from a backup if something happens to your primary server. The common wisdom is if you can avoid building distributed systems, don't build one. Which brings the question if we want to maintain something like this (which will include logic for cases when a node joins or leaves the cluster, proxy server / service discovery for clients, test cases / infrastructure for all this and also upgrading the cluster, docs, ...) for a presumably view users which business doesn't fit in a single server *and* they want an automatic failover (not the manual one) *and* they don't use Patroni/Stolon/CockroachDB/Neon/... already. Although the idea is tempting personally I'm inclined to think that it's better to invest community resources into something else. -- Best regards, Aleksander Alekseev ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 11:14 Konstantin Osipov <[email protected]> parent: Kirill Reshke <[email protected]> 1 sibling, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-15 11:14 UTC (permalink / raw) To: Yura Sokolov <[email protected]>; +Cc: Kirill Reshke <[email protected]>; [email protected] * Yura Sokolov <[email protected]> [25/04/15 12:02]: > > OTOH Raft needs to write its own log, and what's worse, it sometimes > > needs to remove already written parts of it (so, it is not appended > > only, unlike WAL). If you have a production system which maintains two > > kinds of logs with different semantics, it is a very hard system to > > maintain.. > > Raft is log replication protocol which uses log position and term. > But... PostgreSQL already have log position and term in its WAL structure. > PostgreSQL's timeline is actually the Term. > Raft implementer needs just to correct rules for Term/Timeline switching: > - instead of "next TimeLine number is just increment of largest known > TimeLine number" it needs to be "next TimeLine number is the result of > Leader Election". > > And yes, "it sometimes needs to remove already written parts of it". > But... It is exactly what every PostgreSQL's cluster manager software have > to do to join previous leader as a follower to new leader - pg_rewind. > > So, PostgreSQL already have 70-90%% of Raft implementation details. > Raft doesn't have to be implemented in PostgreSQL. > Raft has to be finished!!! > > PS: One of the biggest issues is forced snapshot on replica promotion. It > really slows down leader switch time. It looks like it is not really > needed, or some small workaround should be enough. I'd say my pet peeve is storing the cluster topology (the so called raft configuration) inside the database, not in an external state provider. Agree on other points. -- Konstantin Osipov ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 11:15 Aleksander Alekseev <[email protected]> parent: Aleksander Alekseev <[email protected]> 2 siblings, 1 reply; 33+ messages in thread From: Aleksander Alekseev @ 2025-04-15 11:15 UTC (permalink / raw) To: [email protected]; +Cc: Yura Sokolov <[email protected]>; Konstantin Osipov <[email protected]> Hi Yura, > I've been working in a company which uses MongoDB (3.6 and up) as their > primary storage. And it seemed to me as "God Send". Everything just worked. > Replication was as reliable as one could imagine. It outlives several > hardware incidents without manual intervention. It allowed cluster > maintenance (software and hardware upgrades) without application downtime. > I really dream PostgreSQL will be as reliable as MongoDB without need of > external services. I completely understand. I had exactly the same experience with Stolon. Everything just worked. And the setup took like 5 minutes. It's a pity this project doesn't seem to get as much attention as Patroni. Probably because attention requires traveling and presenting the project at conferences which costs money. Or perhaps people are just happy with Patroni. I'm not sure in which state Stolon is today. -- Best regards, Aleksander Alekseev ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 11:20 Yura Sokolov <[email protected]> parent: Aleksander Alekseev <[email protected]> 0 siblings, 0 replies; 33+ messages in thread From: Yura Sokolov @ 2025-04-15 11:20 UTC (permalink / raw) To: Aleksander Alekseev <[email protected]>; [email protected]; +Cc: Konstantin Osipov <[email protected]> 15.04.2025 14:15, Aleksander Alekseev пишет: > Hi Yura, > >> I've been working in a company which uses MongoDB (3.6 and up) as their >> primary storage. And it seemed to me as "God Send". Everything just worked. >> Replication was as reliable as one could imagine. It outlives several >> hardware incidents without manual intervention. It allowed cluster >> maintenance (software and hardware upgrades) without application downtime. >> I really dream PostgreSQL will be as reliable as MongoDB without need of >> external services. > > I completely understand. I had exactly the same experience with > Stolon. Everything just worked. And the setup took like 5 minutes. > > It's a pity this project doesn't seem to get as much attention as > Patroni. Probably because attention requires traveling and presenting > the project at conferences which costs money. Or perhaps people are > just happy with Patroni. I'm not sure in which state Stolon is today. But the key point: if PostgreSQL will be improved a bit, there will be no need neither in Patroni, nor in Stolon. Isn't it great? -- regards Yura Sokolov aka funny-falcon ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 11:23 Konstantin Osipov <[email protected]> parent: Aleksander Alekseev <[email protected]> 2 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-15 11:23 UTC (permalink / raw) To: Aleksander Alekseev <[email protected]>; +Cc: [email protected] * Aleksander Alekseev <[email protected]> [25/04/15 13:20]: > > I am considering starting work on implementing a built-in Raft > > replication for PostgreSQL. > > Generally speaking I like the idea. The more important question IMO is > whether we want to maintain Raft within the PostgreSQL core project. > > Building distributed systems on commodity hardware was a popular idea > back in the 2000s. These days you can rent a server with 2 Tb of RAM > for something like 2000 USD/month (numbers from my memory that were > valid ~5 years ago) which will fit many of the existing businesses (!) > in memory. And you can rent another one for a replica, just in order > not to recover from a backup if something happens to your primary > server. The common wisdom is if you can avoid building distributed > systems, don't build one. > > Which brings the question if we want to maintain something like this > (which will include logic for cases when a node joins or leaves the > cluster, proxy server / service discovery for clients, test cases / > infrastructure for all this and also upgrading the cluster, docs, ...) > for a presumably view users which business doesn't fit in a single > server *and* they want an automatic failover (not the manual one) > *and* they don't use Patroni/Stolon/CockroachDB/Neon/... already. > > Although the idea is tempting personally I'm inclined to think that > it's better to invest community resources into something else. My personal take away from this as a community member would be seamless coordinator failover in Greenplum and all of its forks (CloudBerry, Greengage, synxdata, what not). I also imagine there is a number of PostgreSQL derivatives that could benefit from built-in transparent failover since it standardizes the solution space. -- Konstantin Osipov ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 11:24 Konstantin Osipov <[email protected]> parent: Aleksander Alekseev <[email protected]> 2 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-15 11:24 UTC (permalink / raw) To: Yura Sokolov <[email protected]>; +Cc: Aleksander Alekseev <[email protected]>; [email protected] * Yura Sokolov <[email protected]> [25/04/15 14:02]: > I've been working in a company which uses MongoDB (3.6 and up) as their > primary storage. And it seemed to me as "God Send". Everything just worked. > Replication was as reliable as one could imagine. It outlives several > hardware incidents without manual intervention. It allowed cluster > maintenance (software and hardware upgrades) without application downtime. > I really dream PostgreSQL will be as reliable as MongoDB without need of > external services. thanks for pointing out mongodb, so built-in raft would help ferretdb as well. -- Konstantin Osipov ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 15:07 Greg Sabino Mullane <[email protected]> parent: Konstantin Osipov <[email protected]> 2 siblings, 2 replies; 33+ messages in thread From: Greg Sabino Mullane @ 2025-04-15 15:07 UTC (permalink / raw) To: Konstantin Osipov <[email protected]>; +Cc: [email protected] On Mon, Apr 14, 2025 at 1:15 PM Konstantin Osipov <[email protected]> wrote: > If anyone is working on Raft already I'd be happy to discuss > the details. I am fairly new to the PostgreSQL hackers ecosystem > so cautious of starting work in isolation/knowing there is no > interest in accepting the feature into the trunk. > Putting aside the technical concerns about this specific idea, it's best to start by laying out a very detailed plan of what you would want to change, and what you see as the costs and benefits. It's also extremely helpful to think about developing this as an extension. If you get stuck due to extension limitations, propose additional hooks. If the hooks will not work, explain why. Getting this into core is going to be a long, multi-year effort, in which people are going to be pushing back the entire time, so prepare yourself for that. My immediate retort is going to be: why would we add this if there are existing tools that already do the job just fine? Postgres has lots of tasks that it is happy to let other programs/OS subsystems/extensions/etc. handle instead. Cheers, Greg -- Crunchy Data - https://www.crunchydata.com Enterprise Postgres Software Products & Tech Support ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 17:27 Konstantin Osipov <[email protected]> parent: Greg Sabino Mullane <[email protected]> 1 sibling, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-15 17:27 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: [email protected] * Greg Sabino Mullane <[email protected]> [25/04/15 18:08]: > > If anyone is working on Raft already I'd be happy to discuss > > the details. I am fairly new to the PostgreSQL hackers ecosystem > > so cautious of starting work in isolation/knowing there is no > > interest in accepting the feature into the trunk. > > > > Putting aside the technical concerns about this specific idea, it's best to > start by laying out a very detailed plan of what you would want to change, > and what you see as the costs and benefits. It's also extremely helpful to > think about developing this as an extension. If you get stuck due to > extension limitations, propose additional hooks. If the hooks will not > work, explain why. > > Getting this into core is going to be a long, multi-year effort, in which > people are going to be pushing back the entire time, so prepare yourself > for that. My immediate retort is going to be: why would we add this if > there are existing tools that already do the job just fine? Postgres has > lots of tasks that it is happy to let other programs/OS > subsystems/extensions/etc. handle instead. I had hoped I explained why external state providers can not provide the same seamless UX as built-in ones. The key idea is to have a built-in configuration management, so that adding and removing replicas does not require changes in multiple disjoint parts of the installation (server configurations, proxies, clients). I understand and accept that it's a multi-year effort, but I do not accept the retort - my main point is that external tools are not a replacement, and I'd like to reach consensus on that. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-15 22:27 Nikolay Samokhvalov <[email protected]> parent: Greg Sabino Mullane <[email protected]> 1 sibling, 8 replies; 33+ messages in thread From: Nikolay Samokhvalov @ 2025-04-15 22:27 UTC (permalink / raw) To: Greg Sabino Mullane <[email protected]>; +Cc: Konstantin Osipov <[email protected]>; [email protected] On Tue, Apr 15, 2025 at 8:08 AM Greg Sabino Mullane <[email protected]> wrote: > On Mon, Apr 14, 2025 at 1:15 PM Konstantin Osipov <[email protected]> > wrote: > >> If anyone is working on Raft already I'd be happy to discuss >> the details. I am fairly new to the PostgreSQL hackers ecosystem >> so cautious of starting work in isolation/knowing there is no >> interest in accepting the feature into the trunk. >> > > Putting aside the technical concerns about this specific idea, it's best > to start by laying out a very detailed plan of what you would want to > change, and what you see as the costs and benefits. It's also extremely > helpful to think about developing this as an extension. If you get stuck > due to extension limitations, propose additional hooks. If the hooks will > not work, explain why. > This is exactly what I wanted to write as well. The idea is great. At the same time, I think, consensus on many decisions will be extremely hard to reach, so this project has a high risk of being very long. Unless it's an extension, at least in the beginning. Nik ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 05:39 Kirill Reshke <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 1 reply; 33+ messages in thread From: Kirill Reshke @ 2025-04-16 05:39 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Tom Lane <[email protected]>; Konstantin Osipov <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, 16 Apr 2025 at 10:25, Andrey Borodin <[email protected]> wrote: > > I think I can provide some reasons why it cannot be neither extension, nor any part running within postmaster reign. > > 1. When joining cluster, there’s not PGDATA to run postmaster on top of it. You can join the cluster on pg_basebackup of its master; So I dont get why this is an anti-extension restriction. > 2. After failover, old Primary node must rejoin cluster by running pg_rewind and following timeline switch. You can run bash from extension, what's the point? > The system in hand must be able to manipulate with PGDATA without starting Postgres. -- Best regards, Kirill Reshke ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 05:44 Andrey Borodin <[email protected]> parent: Kirill Reshke <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Andrey Borodin @ 2025-04-16 05:44 UTC (permalink / raw) To: Kirill Reshke <[email protected]>; +Cc: Tom Lane <[email protected]>; Konstantin Osipov <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> > On 16 Apr 2025, at 10:39, Kirill Reshke <[email protected]> wrote: > > You can run bash from extension, what's the point? You cannot run bash that will stop backend running bash. Best regards, Andrey Borodin. ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 09:47 Konstantin Osipov <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-16 09:47 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; [email protected] * Tom Lane <[email protected]> [25/04/16 11:05]: > Nikolay Samokhvalov <[email protected]> writes: > > This is exactly what I wanted to write as well. The idea is great. At the > > same time, I think, consensus on many decisions will be extremely hard to > > reach, so this project has a high risk of being very long. Unless it's an > > extension, at least in the beginning. > > Yeah. The two questions you'd have to get past to get this into PG > core are: > > 1. Why can't it be an extension? (You claimed it would work more > seamlessly in core, but I don't think you've made a proven case.) I think this can be best addressed when the discussion moves on to an architecture design record, where the UX and implementation details are outlined. I'm sure there can be a lot of bike-shedding on that part. For now I merely wanted to know if: - maybe there is a reason this will never be accepted - maybe someone is already working on this. ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 09:53 Konstantin Osipov <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-16 09:53 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Tom Lane <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; [email protected] * Ashutosh Bapat <[email protected]> [25/04/16 11:06]: > > My view is what Konstantin wants is automatic replication topology management. For some reason this technology is called HA, DCS, Raft, Paxos and many other scary words. But basically it manages primary_conn_info of some nodes to provide some fault-tolerance properties. I'd start to design from here, not from Raft paper. > > > In my experience, the load of managing hundreds of replicas which all > participate in RAFT protocol becomes more than regular transaction > load. So making every replica a RAFT participant will affect the > ability to deploy hundreds of replica. I think this experience needs to be detailed out. There are implementations in the field that are less efficient than others. Early etcd-raft didn't have pre-voting and had "bastardized" (their own definition) implementation of configuration changes which didn't use joint consensus. Then there is a liveness issue if leader election is implemented in a straightforward way in large clusters. But this is addressed: scaling up the randomized election timeout with the cluster size, converting most of participants to non-voters in large clusters. Raft replication, again, if implemented in a naive way, would require a O(outstanding transaction) * number of replicas amount of RAM. But that doesn't have to be naive. To sum up, I am not aware of any principal limitations in this area. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 09:58 Konstantin Osipov <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-16 09:58 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Tom Lane <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> * Andrey Borodin <[email protected]> [25/04/16 11:06]: > > Andrey Borodin <[email protected]> writes: > >> I think it's what Konstantin is proposing. To have our own Raft implementation, without dependencies. > > > > Hmm, OK. I thought that the proposal involved relying on some existing > > code, but re-reading the thread that was said nowhere. Still, that > > moves it from a large project to a really large project :-( > > > > I continue to think that it'd be best to try to implement it as > > an extension, at least up till the point of finding show-stopping > > reasons why it cannot be that. > > I think I can provide some reasons why it cannot be neither extension, nor any part running within postmaster reign. > > 1. When joining cluster, there’s not PGDATA to run postmaster on top of it. > > 2. After failover, old Primary node must rejoin cluster by running pg_rewind and following timeline switch. > > The system in hand must be able to manipulate with PGDATA without starting Postgres. > > My question to Konstantin is Why wouldn’t you just add Raft to Patroni? Is there a reason why something like Patroni is not in core and noone rushes to get it in? > Everyone is using it, or system like it. Raft uses the same WAL to store configuration change records as is used for commit records. This is at the core of the correctness of the algorithm. This is also my biggest concern with correctness of Patroni - but to the best of my knowledge 's 90%+ of use cases of Patroni use a "fixed" quorum size, that's defined at start of the deployment and never/rarely changes. Contrast to that being able to a replica to the quorum at any time, and all it takes is just starting this replica and pointing it at the existing cluster. This greatly simplifies UX. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 10:02 Konstantin Osipov <[email protected]> parent: Andrey Borodin <[email protected]> 0 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-16 10:02 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; +Cc: Kirill Reshke <[email protected]>; Tom Lane <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> * Andrey Borodin <[email protected]> [25/04/16 11:06]: > > You can run bash from extension, what's the point? > > You cannot run bash that will stop backend running bash. You're right there is a chicken and egg problem when you add Raft to an existing project, and rebootstrap becomes a trick, but it's a plumbing trick. The new member needs to generate and persist a globally unique identifier as the first step. Later it can reintroduce itself to the cluster given this identifier can be preserved in the new incarnation (popen + fork). -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 14:07 Konstantin Osipov <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Konstantin Osipov @ 2025-04-16 14:07 UTC (permalink / raw) To: Alastair Turner <[email protected]>; +Cc: Ashutosh Bapat <[email protected]>; Andrey Borodin <[email protected]>; Tom Lane <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> * Alastair Turner <[email protected]> [25/04/16 15:58]: > > > If you use build-in failover you have to resort to 3 big Postgres > > machines because you need 2/3 majority. Of course, you can install > > MySQL-stype arbiter - host that had no real PGDATA, only participates in > > voting. But this is a solution to problem induced by built-in autofailover. > > > > Users find it a waste of resources to deploy 3 big PostgreSQL > > instances just for HA where 2 suffice even if they deploy 3 > > lightweight DCS instances. Having only some of the nodes act as DCS > > and others purely PostgreSQL nodes will reduce waste of resources. > > > > The experience of other projects/products with automated failover based on > quorum shows that this is a critical issue for adoption. In the In-memory > Data Grid space (Coherence, Geode/GemFire) the question of how to ensure > that some nodes didn't carry any data comes up early in many architecture > discussions. When RabbitMQ shipped their Quorum Queues feature, the first > and hardest area of pushback was around all nodes hosting message content. > > It's not just about the requirement for compute resources, it's also about > bandwidth and latency. Many large organisations have, for historical > reasons, pairs of data centres with very good point-to-point connectivity. > As the requirement for quorum witnesses has come up for all sorts of > things, including storage arrays, they have built arbiter/witness sites at > branches, colocation providers or even on the public cloud. More than not > holding user data or processing queries, the arbiter can't even be sent the > replication stream for the user data in the database, it just won't fit > down the pipe. > > Which feels like a very difficult requirement to meet if the replication > model for all data is being changed to a quorum model. I agree master/replica deployment layouts are very popular and are not going to directly benefit from raft. They'll still work, but no automation will be available, just like today with Patroni. However, if the storage cost is an argument, then the logical path is to disaggregate storage/compute altogether, i.e. use projects like neon. -- Konstantin Osipov, Moscow, Russia ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 14:29 Yura Sokolov <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Yura Sokolov @ 2025-04-16 14:29 UTC (permalink / raw) To: Andrey Borodin <[email protected]>; Tom Lane <[email protected]>; +Cc: Konstantin Osipov <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> 16.04.2025 08:24, Andrey Borodin пишет: > 2. After failover, old Primary node must rejoin cluster by running pg_rewind and following timeline switch. It is really do-able: BiHA already does it. And BiHA runs as a child process of postmaster, ie both postmaster and BiHA doesn't restart when PostgreSQL needs to rewind and restart. Yes, there are non-trivial amount of changes made into postmaster machinery. But it is doable. -- regards Yura Sokolov aka funny-falcon ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 19:29 Greg Sabino Mullane <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 1 reply; 33+ messages in thread From: Greg Sabino Mullane @ 2025-04-16 19:29 UTC (permalink / raw) To: Ashutosh Bapat <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Tom Lane <[email protected]>; Konstantin Osipov <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> On Wed, Apr 16, 2025 at 2:18 AM Ashutosh Bapat <[email protected]> wrote: > Users find it a waste of resources to deploy 3 big PostgreSQL instances > just for HA where 2 suffice even if they deploy 3 lightweight DCS > instances. Having only some of the nodes act as DCS and others purely > PostgreSQL nodes will reduce waste of resources. > A big problem is that putting your DCS into Postgres means your whole system is now super-sensitive to IO/WAL-streaming issues, and a busy database doing database stuff is going to start affecting the DCS stuff. With three lightweight DCS servers, you don't really need to worry about how stressed the database servers are. In that way, I feel etcd et al. are adhering to the unix philosophy of "do one thing, and do it well." Cheers, Greg -- Crunchy Data - https://www.crunchydata.com Enterprise Postgres Software Products & Tech Support ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-16 21:24 Hannu Krosing <[email protected]> parent: Nikolay Samokhvalov <[email protected]> 7 siblings, 0 replies; 33+ messages in thread From: Hannu Krosing @ 2025-04-16 21:24 UTC (permalink / raw) To: Tom Lane <[email protected]>; +Cc: Andrey Borodin <[email protected]>; Konstantin Osipov <[email protected]>; Greg Sabino Mullane <[email protected]>; Nikolay Samokhvalov <[email protected]>; [email protected] On Wed, Apr 16, 2025 at 6:27 AM Tom Lane <[email protected]> wrote: > > Andrey Borodin <[email protected]> writes: > > I think it's what Konstantin is proposing. To have our own Raft implementation, without dependencies. > > Hmm, OK. I thought that the proposal involved relying on some existing > code, but re-reading the thread that was said nowhere. Still, that > moves it from a large project to a really large project :-( My understanding is that RAFT is a fancy name for what PostgreSQL is largely already doing - "electing" a leader and then doing all the changes through that leader (a.k.a. WAL streaming) The thing that needs adding - and which makes it "RAFT" instead of just a streaming replication with a failover - is what happens when the leader goes away and one of the replicas needs to become a new leader. We have ways to do rollbacks and roll-forwards but the main tricky part is "how do you know that you have not lost some changes" and here we must have some place to store the info about at which LSN the failover happened, so that we know to run pg_rewind if any losts hosts come back and want to join. And of course we need to have a way to communicate this "who is the new leader" to clients which needs new libpgq functionality of "failover connection pools" which hide the failovers from old clients. The RAFT protocol could be a provider of "who is current leader info" and optionally cache the LSN the switch happened. > I continue to think that it'd be best to try to implement it as > an extension, at least up till the point of finding show-stopping > reasons why it cannot be that. The main thing I would like to see in core is ability to do clean *switchovers* (not failovers) by sending a magic WAL record with a message "hey node N, please take over the write node role" over WAL so that node N knows to self-promote and all other nodes know to start following N starting from the next WAL record either directly or Why WAL - because it is already is sent to all replicas, it guarantees continuity as it very clearly states at what LSN the write-head-switch happens. We also should be able to send this info to the client libraries currently connected to the writer. so that they can choose to switch to the new head. The rest could be easily an extension. Mainly we want more than one "coordinators" which can be running in some or all of the nodes.and who agree on - which node is current leader - at which LSN the switch happened (so if some node coming back discovers that it has magicall moved ahead it knows to rewind to that LSN and then re-stream it from commonly agreed place. It would also be nice to have some agreed, hopefully lightweight, notion of node identity, which we could then use for many things, including stamping it in WAL records to guarantee / verify that all the nodes have been on the same WAL stream all the time But regarding weather to use RAFT I would just define a "coordinator API" and leave it up to the specific coordinator/consensus extension to decide how the consensus is achieved So to summarize: # Core should provide - way tomove to new node, - for switchover a WAL-based switchover - for failover something similar which also writes the WAL record so all histories are synced - a libpq message informing clients about "new write head node" - node IDs and more general c;luster-awareness inside the PostgreSQL node (I had a shoutout about this in a recent pgconf.dev unconference talk) - a new write-node field in WAL to track write head movement - API for a joining node to find out which cluster it joins and the switchover history - in WAL it is always switchover, maybe with some info saying that it was a forces switchover because we lost old write head - if some lost node comes back it may need to rewind or re-initialize if it finds out it had been following a lost timeline that is not fully part of NOTE: switchovers in WAL would be very similar to timeline changes. I am not sure how much extra info is needed there. # Extension can provide - agreeing on new leader node in case of failover - protocol can be RAFT, PAXOS or "the DBA says so" :) - sharing fresh info about current leader and switch timelines (though this should more likely be in core) - anything else ??? # external apps is (likely?) needed for - setting up cluster, provisioning machines / VMs - setting up networking - starting PostgreSQL servers. - spinning up and down clients, - communicating current leader and replica set (could be done by DNS with agreed conventions) ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: Built-in Raft replication @ 2025-04-29 20:16 Jim Nasby <[email protected]> parent: Greg Sabino Mullane <[email protected]> 0 siblings, 0 replies; 33+ messages in thread From: Jim Nasby @ 2025-04-29 20:16 UTC (permalink / raw) To: Devrim Gündüz <[email protected]>; +Cc: Greg Sabino Mullane <[email protected]>; Ashutosh Bapat <[email protected]>; Andrey Borodin <[email protected]>; Tom Lane <[email protected]>; Konstantin Osipov <[email protected]>; Nikolay Samokhvalov <[email protected]>; PostgreSQL Hackers <[email protected]> I've always assumed there'd have to be at least one global stream, if for no other purpose than to be the source of truth about transaction commit ordering (though, I was thinking of supporting multiple streams for one database). Presumably the same could be used for shared objects. Or perhaps shared objects just get their own stream. Either way, having a master commit record that points at LSNs of various other streams is what I'd been thinking. On Wed, Apr 23, 2025 at 12:01 PM Devrim Gündüz <[email protected]> wrote: > Hi, > > On Wed, 2025-04-23 at 11:48 -0500, Jim Nasby wrote: > > unless we added multiple WAL streams. That would allow for splitting > > WAL traffic across multiple devices as well as providing better > > support for configurations that don’t replicate the entire cluster. > > The current situation where delayed replication of a single table > > mandates retention of all the WAL for the entire cluster is less than > > ideal. > > I think the problem is handling the stream of global objects. Having > separate stream for each database would be awesome as long as it can > deal with the "global stream". > > Regards, > -- > Devrim Gündüz > Open Source Solution Architect, PostgreSQL Major Contributor > BlueSky: @devrim.gunduz.org , @gunduz.org > ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 05:03 Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Etsuro Fujita @ 2026-07-10 05:03 UTC (permalink / raw) To: Corey Huinker <[email protected]>; +Cc: Robert Haas <[email protected]>; pgsql-hackers On Wed, Jul 8, 2026 at 9:04 PM Etsuro Fujita <[email protected]> wrote: > Attached is a new version > of the patch. I'm planning to push and back-patch it, if no > objections. Committed and back-patched. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 05:36 Fujii Masao <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Fujii Masao @ 2026-07-10 05:36 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On Fri, Jul 10, 2026 at 2:03 PM Etsuro Fujita <[email protected]> wrote: > > On Wed, Jul 8, 2026 at 9:04 PM Etsuro Fujita <[email protected]> wrote: > > Attached is a new version > > of the patch. I'm planning to push and back-patch it, if no > > objections. > > Committed and back-patched. In the committed patch, I found that the set_*_arg() helper functions in postgres_fdw.c are declared static in their prototypes, but their definitions omit the static keyword. This looks like an oversight. Attached patch adds static to the definitions for consistency and to make their intended file-local scope explicit. It also fixes a few nearby comment typos. Regards, -- Fujii Masao Attachments: [application/octet-stream] v1-0001-postgres_fdw-Mark-statistics-import-helpers-as-st.patch (3.4K, ../../CAHGQGwGjcQ4SwHMUQ9P8UYQ7iLKL1QE3uLSdONToQ1MrzpUUoQ@mail.gmail.com/2-v1-0001-postgres_fdw-Mark-statistics-import-helpers-as-st.patch) download | inline diff: From f9793e5fe924b49e1cad93f7d33ed313283c51f7 Mon Sep 17 00:00:00 2001 From: Fujii Masao <[email protected]> Date: Fri, 10 Jul 2026 14:22:54 +0900 Subject: [PATCH v1] postgres_fdw: Mark statistics import helpers as static The set_*_arg helper functions in postgres_fdw.c are declared static, but their definitions omitted the static keyword. Add it to make their file-local scope explicit and keep the declarations and definitions consistent. Also fix a couple of nearby comment typos. --- contrib/postgres_fdw/postgres_fdw.c | 12 ++++++------ src/backend/statistics/attribute_stats.c | 2 +- src/backend/statistics/relation_stats.c | 2 +- 3 files changed, 8 insertions(+), 8 deletions(-) diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c index e6e0a4ab9b5..70de942de12 100644 --- a/contrib/postgres_fdw/postgres_fdw.c +++ b/contrib/postgres_fdw/postgres_fdw.c @@ -6134,7 +6134,7 @@ import_fetched_statistics(Relation relation, } /* - * Conenience routine to fetch the value for the row/column of the PGresult + * Convenience routine to fetch the value for the row/column of the PGresult */ static char * get_opt_value(PGresult *res, int row, int col) @@ -6147,7 +6147,7 @@ get_opt_value(PGresult *res, int row, int col) /* * Convenience routine for setting optional text arguments */ -void +static void set_text_arg(NullableDatum *arg, const char *s) { if (s) @@ -6165,7 +6165,7 @@ set_text_arg(NullableDatum *arg, const char *s) /* * Convenience routine for setting optional int32 arguments */ -void +static void set_int32_arg(NullableDatum *arg, const char *s) { if (s) @@ -6185,7 +6185,7 @@ set_int32_arg(NullableDatum *arg, const char *s) /* * Convenience routine for setting optional uint32 arguments */ -void +static void set_uint32_arg(NullableDatum *arg, const char *s) { if (s) @@ -6205,7 +6205,7 @@ set_uint32_arg(NullableDatum *arg, const char *s) /* * Convenience routine for setting optional float arguments */ -void +static void set_float_arg(NullableDatum *arg, const char *s) { if (s) @@ -6225,7 +6225,7 @@ set_float_arg(NullableDatum *arg, const char *s) /* * Convenience routine for setting optional float[] arguments */ -void +static void set_floatarr_arg(NullableDatum *arg, const char *s) { if (s) diff --git a/src/backend/statistics/attribute_stats.c b/src/backend/statistics/attribute_stats.c index 2efb74b95a6..48e0df391e2 100644 --- a/src/backend/statistics/attribute_stats.c +++ b/src/backend/statistics/attribute_stats.c @@ -708,7 +708,7 @@ pg_restore_attribute_stats(PG_FUNCTION_ARGS) } /* - * Import attribute statistics from NullableDatum inputs for all statitical + * Import attribute statistics from NullableDatum inputs for all statistical * values. * * For now, the 'version' argument is ignored. In the future it can be used diff --git a/src/backend/statistics/relation_stats.c b/src/backend/statistics/relation_stats.c index 990a7511d04..fbaab92284f 100644 --- a/src/backend/statistics/relation_stats.c +++ b/src/backend/statistics/relation_stats.c @@ -256,7 +256,7 @@ pg_restore_relation_stats(PG_FUNCTION_ARGS) } /* - * Import relation statistics from NullableDatum inputs for all statitical + * Import relation statistics from NullableDatum inputs for all statistical * values. * * For now, the 'version' argument is ignored. In the future it can be used -- 2.55.0 ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 07:40 Etsuro Fujita <[email protected]> parent: Fujii Masao <[email protected]> 0 siblings, 1 reply; 33+ messages in thread From: Etsuro Fujita @ 2026-07-10 07:40 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers Fujii-san, On Fri, Jul 10, 2026 at 2:37 PM Fujii Masao <[email protected]> wrote: > In the committed patch, I found that the set_*_arg() helper functions > in postgres_fdw.c are declared static in their prototypes, but their > definitions omit the static keyword. This looks like an oversight. > > Attached patch adds static to the definitions for consistency and > to make their intended file-local scope explicit. It also fixes a few > nearby comment typos. Good catch! Thanks for the patch! LGTM. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 11:36 Etsuro Fujita <[email protected]> parent: Etsuro Fujita <[email protected]> 0 siblings, 2 replies; 33+ messages in thread From: Etsuro Fujita @ 2026-07-10 11:36 UTC (permalink / raw) To: Fujii Masao <[email protected]>; +Cc: Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On Fri, Jul 10, 2026 at 4:40 PM Etsuro Fujita <[email protected]> wrote: > On Fri, Jul 10, 2026 at 2:37 PM Fujii Masao <[email protected]> wrote: > > In the committed patch, I found that the set_*_arg() helper functions > > in postgres_fdw.c are declared static in their prototypes, but their > > definitions omit the static keyword. This looks like an oversight. > > > > Attached patch adds static to the definitions for consistency and > > to make their intended file-local scope explicit. It also fixes a few > > nearby comment typos. > > Good catch! Thanks for the patch! LGTM. My compiler (Apple clang version 17.0.0 (clang-1700.4.4.1)) doesn't output any error/warning about that declaration. Actually, it's allowed? Anyway, +1 for adding it for consistency. Best regards, Etsuro Fujita ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 12:09 Fujii Masao <[email protected]> parent: Etsuro Fujita <[email protected]> 1 sibling, 0 replies; 33+ messages in thread From: Fujii Masao @ 2026-07-10 12:09 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers On Fri, Jul 10, 2026 at 8:37 PM Etsuro Fujita <[email protected]> wrote: > > On Fri, Jul 10, 2026 at 4:40 PM Etsuro Fujita <[email protected]> wrote: > > On Fri, Jul 10, 2026 at 2:37 PM Fujii Masao <[email protected]> wrote: > > > In the committed patch, I found that the set_*_arg() helper functions > > > in postgres_fdw.c are declared static in their prototypes, but their > > > definitions omit the static keyword. This looks like an oversight. > > > > > > Attached patch adds static to the definitions for consistency and > > > to make their intended file-local scope explicit. It also fixes a few > > > nearby comment typos. > > > > Good catch! Thanks for the patch! LGTM. > > My compiler (Apple clang version 17.0.0 (clang-1700.4.4.1)) doesn't > output any error/warning about that declaration. Actually, it's > allowed? Anyway, +1 for adding it for consistency. Maybe that's allowed. I wonder adding static to both the declaration and the definition seems mainly a matter of consistency, readability, and coding style. Thanks for the review! I've pushed the patch. Regards, -- Fujii Masao ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 14:08 Tom Lane <[email protected]> parent: Etsuro Fujita <[email protected]> 1 sibling, 1 reply; 33+ messages in thread From: Tom Lane @ 2026-07-10 14:08 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Fujii Masao <[email protected]>; Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers Etsuro Fujita <[email protected]> writes: > On Fri, Jul 10, 2026 at 4:40 PM Etsuro Fujita <[email protected]> wrote: >> On Fri, Jul 10, 2026 at 2:37 PM Fujii Masao <[email protected]> wrote: >>> Attached patch adds static to the definitions for consistency and >>> to make their intended file-local scope explicit. It also fixes a few >>> nearby comment typos. > My compiler (Apple clang version 17.0.0 (clang-1700.4.4.1)) doesn't > output any error/warning about that declaration. Actually, it's > allowed? Anyway, +1 for adding it for consistency. The back story here is that that coding pattern is allowed by standard C but traditionally (pre-ANSI C or so) wasn't. We used to have some buildfarm animals that would warn about it, but none do today. I think it's a good idea to include "static" in the definitions for clarity, and so that you don't have to go looking for the declaration to know if a function is file-local or not. But the standard doesn't require that. Rummaging in the gcc manual, it looks like you can turn on a warning for this with '-Wtraditional', but that also enables a boatload of warnings we don't want, so I can't see using it on a regular basis. regards, tom lane ^ permalink raw reply [nested|flat] 33+ messages in thread
* Re: use of SPI by postgresImportForeignStatistics @ 2026-07-10 14:41 Tom Lane <[email protected]> parent: Tom Lane <[email protected]> 0 siblings, 0 replies; 33+ messages in thread From: Tom Lane @ 2026-07-10 14:41 UTC (permalink / raw) To: Etsuro Fujita <[email protected]>; +Cc: Fujii Masao <[email protected]>; Corey Huinker <[email protected]>; Robert Haas <[email protected]>; pgsql-hackers I wrote: > Rummaging in the gcc manual, it looks like you can turn on a warning > for this with '-Wtraditional', but that also enables a boatload of > warnings we don't want, so I can't see using it on a regular basis. I experimented with that for fun. On current HEAD, it produces 347111 warnings, mostly "traditional C rejects ISO C style function definitions". But amongst the dross there's dbcommands.c:395:1: warning: non-static declaration of 'ScanSourceDatabasePgClassTuple' follows static declaration [-Wtraditional] event_trigger.c:392:1: warning: non-static declaration of 'SetDatabaseHasLoginEventTriggers' follows static declaration [-Wtraditional] tablecmds.c:20846:1: warning: non-static declaration of 'ConstraintImpliedByRelConstraint' follows static declaration [-Wtraditional] nodeModifyTable.c:4154:1: warning: non-static declaration of 'ExecInitMerge' follows static declaration [-Wtraditional] postmaster.c:4056:1: warning: non-static declaration of 'StartSysLogger' follows static declaration [-Wtraditional] buf_table.c:48:1: warning: non-static declaration of 'BufTableShmemRequest' follows static declaration [-Wtraditional] oracle_compat.c:555:1: warning: non-static declaration of 'dobyteatrim' follows static declaration [-Wtraditional] pg_locale_icu.c:746:1: warning: non-static declaration of 'strncoll_icu_utf8' follows static declaration [-Wtraditional] pg_locale_icu.c:767:1: warning: non-static declaration of 'strcoll_icu_utf8' follows static declaration [-Wtraditional] pg_walsummary.c:230:1: warning: non-static declaration of 'walsummary_error_callback' follows static declaration [-Wtraditional] pg_walsummary.c:245:1: warning: non-static declaration of 'walsummary_read_callback' follows static declaration [-Wtraditional] vacuumdb.c:327:1: warning: non-static declaration of 'check_objfilter' follows static declaration [-Wtraditional] if anyone cares to follow that up. regards, tom lane ^ permalink raw reply [nested|flat] 33+ messages in thread
end of thread, other threads:[~2026-07-10 14:41 UTC | newest] Thread overview: 33+ messages (download: mbox mbox.gz follow: Atom feed) -- links below jump to the message on this page -- 2024-03-29 17:25 Re: psql's FETCH_COUNT (cursor) is not being respected for CTEs Laurenz Albe <[email protected]> 2024-04-01 17:52 ` Daniel Verite <[email protected]> 2024-04-02 16:50 ` Tom Lane <[email protected]> 2025-04-14 17:15 Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-14 17:44 ` Re: Built-in Raft replication Kirill Reshke <[email protected]> 2025-04-14 19:00 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-15 11:14 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-15 10:20 ` Re: Built-in Raft replication Aleksander Alekseev <[email protected]> 2025-04-15 11:15 ` Re: Built-in Raft replication Aleksander Alekseev <[email protected]> 2025-04-15 11:20 ` Re: Built-in Raft replication Yura Sokolov <[email protected]> 2025-04-15 11:23 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-15 11:24 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-15 15:07 ` Re: Built-in Raft replication Greg Sabino Mullane <[email protected]> 2025-04-15 17:27 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-15 22:27 ` Re: Built-in Raft replication Nikolay Samokhvalov <[email protected]> 2025-04-16 05:39 ` Re: Built-in Raft replication Kirill Reshke <[email protected]> 2025-04-16 05:44 ` Re: Built-in Raft replication Andrey Borodin <[email protected]> 2025-04-16 10:02 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-16 09:47 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-16 09:53 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-16 09:58 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-16 14:07 ` Re: Built-in Raft replication Konstantin Osipov <[email protected]> 2025-04-16 14:29 ` Re: Built-in Raft replication Yura Sokolov <[email protected]> 2025-04-16 19:29 ` Re: Built-in Raft replication Greg Sabino Mullane <[email protected]> 2025-04-29 20:16 ` Re: Built-in Raft replication Jim Nasby <[email protected]> 2025-04-16 21:24 ` Re: Built-in Raft replication Hannu Krosing <[email protected]> 2026-07-10 05:03 Re: use of SPI by postgresImportForeignStatistics Etsuro Fujita <[email protected]> 2026-07-10 05:36 ` Re: use of SPI by postgresImportForeignStatistics Fujii Masao <[email protected]> 2026-07-10 07:40 ` Re: use of SPI by postgresImportForeignStatistics Etsuro Fujita <[email protected]> 2026-07-10 11:36 ` Re: use of SPI by postgresImportForeignStatistics Etsuro Fujita <[email protected]> 2026-07-10 12:09 ` Re: use of SPI by postgresImportForeignStatistics Fujii Masao <[email protected]> 2026-07-10 14:08 ` Re: use of SPI by postgresImportForeignStatistics Tom Lane <[email protected]> 2026-07-10 14:41 ` Re: use of SPI by postgresImportForeignStatistics Tom Lane <[email protected]>
This inbox is served by agora; see mirroring instructions for how to clone and mirror all data and code used for this inbox