public inbox for [email protected]  

help / color / mirror / Atom feed

[PATCH 2/2] Fast COPY FROM into the foreign or sharded table.
8+ messages / 5 participants
[nested] [flat]

* [PATCH 2/2] Fast COPY FROM into the foreign or sharded table.
@ 2020-10-19 11:24  Andrey V. Lepikhov <[email protected]>
  0 siblings, 0 replies; 8+ messages in thread

From: Andrey V. Lepikhov @ 2020-10-19 11:24 UTC (permalink / raw)

This feature enables bulk COPY into foreign table in the case of
multi inserts is possible and foreign table has non-zero number of columns.

FDWAPI was extended by next routines:
* BeginForeignCopy
* EndForeignCopy
* ExecForeignCopy

BeginForeignCopy and EndForeignCopy initialize and free
the CopyState of bulk COPY. The ExecForeignCopy routine send
'COPY ... FROM STDIN' command to the foreign server, in iterative
manner send tuples by CopyTo() machinery, send EOF to this connection.

Code that constructed list of columns for a given foreign relation
in the deparseAnalyzeSql() routine is separated to the deparseRelColumnList().
It is reused in the deparseCopyFromSql().

Added TAP-tests on the specific corner cases of COPY FROM STDIN operation.

By the analogy of CopyFrom() the CopyState structure was extended
with data_dest_cb callback. It is used for send text representation
of a tuple to a custom destination.
The PgFdwModifyState structure is extended with the cstate field.
It is needed for avoid repeated initialization of CopyState. ALso for this
reason CopyTo() routine was split into the set of routines CopyToStart()/
CopyTo()/CopyToFinish().

Enum CopyInsertMethod removed. This logic implements by ri_usesMultiInsert
field of the ResultRelInfo sructure.

Discussion:
https://www.postgresql.org/message-id/flat/3d0909dc-3691-a576-208a-90986e55489f%40postgrespro.ru

Authors: Andrey Lepikhov, Ashutosh Bapat, Amit Langote
---
 contrib/postgres_fdw/deparse.c                |  60 ++++-
 .../postgres_fdw/expected/postgres_fdw.out    |  46 +++-
 contrib/postgres_fdw/postgres_fdw.c           | 137 ++++++++++
 contrib/postgres_fdw/postgres_fdw.h           |   1 +
 contrib/postgres_fdw/sql/postgres_fdw.sql     |  45 ++++
 doc/src/sgml/fdwhandler.sgml                  |  73 ++++++
 src/backend/commands/copy.c                   | 236 +++++++++++-------
 src/backend/executor/execMain.c               |   8 +-
 src/backend/executor/execPartition.c          |  29 ++-
 src/include/commands/copy.h                   |  11 +
 src/include/foreign/fdwapi.h                  |  15 ++
 11 files changed, 554 insertions(+), 107 deletions(-)

diff --git a/contrib/postgres_fdw/deparse.c b/contrib/postgres_fdw/deparse.c
index 2d44df19fe..fa7740163d 100644
--- a/contrib/postgres_fdw/deparse.c
+++ b/contrib/postgres_fdw/deparse.c
@@ -184,6 +184,8 @@ static void appendAggOrderBy(List *orderList, List *targetList,
 static void appendFunctionName(Oid funcid, deparse_expr_cxt *context);
 static Node *deparseSortGroupClause(Index ref, List *tlist, bool force_colno,
 									deparse_expr_cxt *context);
+static List *deparseRelColumnList(StringInfo buf, Relation rel,
+								  bool enclose_in_parens);
 
 /*
  * Helper functions
@@ -1758,6 +1760,20 @@ deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
 						 withCheckOptionList, returningList, retrieved_attrs);
 }
 
+/*
+ * Deparse COPY FROM into given buf.
+ * We need to use list of parameters at each query.
+ */
+void
+deparseCopyFromSql(StringInfo buf, Relation rel)
+{
+	appendStringInfoString(buf, "COPY ");
+	deparseRelation(buf, rel);
+	(void) deparseRelColumnList(buf, rel, true);
+
+	appendStringInfoString(buf, " FROM STDIN ");
+}
+
 /*
  * deparse remote UPDATE statement
  *
@@ -2061,6 +2077,30 @@ deparseAnalyzeSizeSql(StringInfo buf, Relation rel)
  */
 void
 deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
+{
+	appendStringInfoString(buf, "SELECT ");
+	*retrieved_attrs = deparseRelColumnList(buf, rel, false);
+
+	/* Don't generate bad syntax for zero-column relation. */
+	if (list_length(*retrieved_attrs) == 0)
+		appendStringInfoString(buf, "NULL");
+
+	/*
+	 * Construct FROM clause
+	 */
+	appendStringInfoString(buf, " FROM ");
+	deparseRelation(buf, rel);
+}
+
+/*
+ * Construct the list of columns of given foreign relation in the order they
+ * appear in the tuple descriptor of the relation. Ignore any dropped columns.
+ * Use column names on the foreign server instead of local names.
+ *
+ * Optionally enclose the list in parantheses.
+ */
+static List *
+deparseRelColumnList(StringInfo buf, Relation rel, bool enclose_in_parens)
 {
 	Oid			relid = RelationGetRelid(rel);
 	TupleDesc	tupdesc = RelationGetDescr(rel);
@@ -2069,10 +2109,8 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
 	List	   *options;
 	ListCell   *lc;
 	bool		first = true;
+	List	   *retrieved_attrs = NIL;
 
-	*retrieved_attrs = NIL;
-
-	appendStringInfoString(buf, "SELECT ");
 	for (i = 0; i < tupdesc->natts; i++)
 	{
 		/* Ignore dropped columns. */
@@ -2081,6 +2119,9 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
 
 		if (!first)
 			appendStringInfoString(buf, ", ");
+		else if (enclose_in_parens)
+			appendStringInfoChar(buf, '(');
+
 		first = false;
 
 		/* Use attribute name or column_name option. */
@@ -2100,18 +2141,13 @@ deparseAnalyzeSql(StringInfo buf, Relation rel, List **retrieved_attrs)
 
 		appendStringInfoString(buf, quote_identifier(colname));
 
-		*retrieved_attrs = lappend_int(*retrieved_attrs, i + 1);
+		retrieved_attrs = lappend_int(retrieved_attrs, i + 1);
 	}
 
-	/* Don't generate bad syntax for zero-column relation. */
-	if (first)
-		appendStringInfoString(buf, "NULL");
+	if (enclose_in_parens && list_length(retrieved_attrs) > 0)
+		appendStringInfoChar(buf, ')');
 
-	/*
-	 * Construct FROM clause
-	 */
-	appendStringInfoString(buf, " FROM ");
-	deparseRelation(buf, rel);
+	return retrieved_attrs;
 }
 
 /*
diff --git a/contrib/postgres_fdw/expected/postgres_fdw.out b/contrib/postgres_fdw/expected/postgres_fdw.out
index 2d88d06358..be8db5ac63 100644
--- a/contrib/postgres_fdw/expected/postgres_fdw.out
+++ b/contrib/postgres_fdw/expected/postgres_fdw.out
@@ -8076,8 +8076,9 @@ copy rem2 from stdin;
 copy rem2 from stdin; -- ERROR
 ERROR:  new row for relation "loc2" violates check constraint "loc2_f1positive"
 DETAIL:  Failing row contains (-1, xyzzy).
-CONTEXT:  remote SQL command: INSERT INTO public.loc2(f1, f2) VALUES ($1, $2)
-COPY rem2, line 1: "-1	xyzzy"
+CONTEXT:  COPY loc2, line 1: "-1	xyzzy"
+remote SQL command: COPY public.loc2(f1, f2) FROM STDIN 
+COPY rem2, line 2
 select * from rem2;
  f1 | f2  
 ----+-----
@@ -8088,6 +8089,19 @@ select * from rem2;
 alter foreign table rem2 drop constraint rem2_f1positive;
 alter table loc2 drop constraint loc2_f1positive;
 delete from rem2;
+create table foo (a int) partition by list (a);
+create table foo1 (like foo);
+create foreign table ffoo1 partition of foo for values in (1)
+	server loopback options (table_name 'foo1');
+create table foo2 (like foo);
+create foreign table ffoo2 partition of foo for values in (2)
+	server loopback options (table_name 'foo2');
+create function print_new_row() returns trigger language plpgsql as $$
+	begin raise notice '%', new; return new; end; $$;
+create trigger ffoo1_br_trig before insert on ffoo1
+	for each row execute function print_new_row();
+copy foo from stdin;
+NOTICE:  (1)
 -- Test local triggers
 create trigger trig_stmt_before before insert on rem2
 	for each statement execute procedure trigger_func();
@@ -8196,6 +8210,34 @@ drop trigger rem2_trig_row_before on rem2;
 drop trigger rem2_trig_row_after on rem2;
 drop trigger loc2_trig_row_before_insert on loc2;
 delete from rem2;
+alter table loc2 drop column f1;
+alter table loc2 drop column f2;
+copy rem2 from stdin;
+ERROR:  column "f1" of relation "loc2" does not exist
+CONTEXT:  remote SQL command: COPY public.loc2(f1, f2) FROM STDIN 
+COPY rem2, line 3
+alter table loc2 add column f1 int;
+alter table loc2 add column f2 int;
+select * from rem2;
+ f1 | f2 
+----+----
+(0 rows)
+
+-- dropped columns locally and on the foreign server
+alter table rem2 drop column f1;
+alter table rem2 drop column f2;
+copy rem2 from stdin;
+select * from rem2;
+--
+(2 rows)
+
+alter table loc2 drop column f1;
+alter table loc2 drop column f2;
+copy rem2 from stdin;
+select * from rem2;
+--
+(4 rows)
+
 -- test COPY FROM with foreign table created in the same transaction
 create table loc3 (f1 int, f2 text);
 begin;
diff --git a/contrib/postgres_fdw/postgres_fdw.c b/contrib/postgres_fdw/postgres_fdw.c
index 9c5aaacc51..1657a20d9b 100644
--- a/contrib/postgres_fdw/postgres_fdw.c
+++ b/contrib/postgres_fdw/postgres_fdw.c
@@ -18,6 +18,7 @@
 #include "access/sysattr.h"
 #include "access/table.h"
 #include "catalog/pg_class.h"
+#include "commands/copy.h"
 #include "commands/defrem.h"
 #include "commands/explain.h"
 #include "commands/vacuum.h"
@@ -190,6 +191,7 @@ typedef struct PgFdwModifyState
 	/* for update row movement if subplan result rel */
 	struct PgFdwModifyState *aux_fmstate;	/* foreign-insert state, if
 											 * created */
+	CopyState cstate; /* foreign COPY state, if used */
 } PgFdwModifyState;
 
 /*
@@ -356,6 +358,13 @@ static void postgresBeginForeignInsert(ModifyTableState *mtstate,
 									   ResultRelInfo *resultRelInfo);
 static void postgresEndForeignInsert(EState *estate,
 									 ResultRelInfo *resultRelInfo);
+static void postgresBeginForeignCopy(ModifyTableState *mtstate,
+									   ResultRelInfo *resultRelInfo);
+static void postgresEndForeignCopy(EState *estate,
+									 ResultRelInfo *resultRelInfo);
+static void postgresExecForeignCopy(ResultRelInfo *resultRelInfo,
+									  TupleTableSlot **slots,
+									  int nslots);
 static int	postgresIsForeignRelUpdatable(Relation rel);
 static bool postgresPlanDirectModify(PlannerInfo *root,
 									 ModifyTable *plan,
@@ -534,6 +543,9 @@ postgres_fdw_handler(PG_FUNCTION_ARGS)
 	routine->EndForeignModify = postgresEndForeignModify;
 	routine->BeginForeignInsert = postgresBeginForeignInsert;
 	routine->EndForeignInsert = postgresEndForeignInsert;
+	routine->BeginForeignCopy = postgresBeginForeignCopy;
+	routine->EndForeignCopy = postgresEndForeignCopy;
+	routine->ExecForeignCopy = postgresExecForeignCopy;
 	routine->IsForeignRelUpdatable = postgresIsForeignRelUpdatable;
 	routine->PlanDirectModify = postgresPlanDirectModify;
 	routine->BeginDirectModify = postgresBeginDirectModify;
@@ -2051,6 +2063,131 @@ postgresEndForeignInsert(EState *estate,
 	finish_foreign_modify(fmstate);
 }
 
+static PgFdwModifyState *copy_fmstate = NULL;
+
+static void
+pgfdw_copy_dest_cb(void *buf, int len)
+{
+	PGconn *conn = copy_fmstate->conn;
+
+	if (PQputCopyData(conn, (char *) buf, len) <= 0)
+		pgfdw_report_error(ERROR, NULL, conn, false, copy_fmstate->query);
+}
+
+/*
+ *
+ * postgresBeginForeignCopy
+ *		Begin an COPY operation on a foreign table
+ */
+static void
+postgresBeginForeignCopy(ModifyTableState *mtstate,
+						   ResultRelInfo *resultRelInfo)
+{
+	PgFdwModifyState *fmstate;
+	StringInfoData sql;
+	RangeTblEntry *rte;
+	Relation rel = resultRelInfo->ri_RelationDesc;
+
+	rte = exec_rt_fetch(resultRelInfo->ri_RangeTableIndex, mtstate->ps.state);
+	initStringInfo(&sql);
+	deparseCopyFromSql(&sql, rel);
+
+	fmstate = create_foreign_modify(mtstate->ps.state,
+									rte,
+									resultRelInfo,
+									CMD_INSERT,
+									NULL,
+									sql.data,
+									NIL,
+									false,
+									NIL);
+
+	fmstate->cstate = BeginCopyTo(NULL, NULL, RelationGetDescr(rel), NULL,
+								  InvalidOid, NULL, false, pgfdw_copy_dest_cb,
+								  NIL, NIL);
+	CopyToStart(fmstate->cstate);
+	resultRelInfo->ri_FdwState = fmstate;
+}
+
+/*
+ * postgresEndForeignCopy
+ *		Finish an COPY operation on a foreign table
+ */
+static void
+postgresEndForeignCopy(EState *estate, ResultRelInfo *resultRelInfo)
+{
+	PgFdwModifyState *fmstate = (PgFdwModifyState *) resultRelInfo->ri_FdwState;
+
+	/* Check correct use of CopyIn FDW API. */
+	Assert(fmstate->cstate != NULL);
+	CopyToFinish(fmstate->cstate);
+	pfree(fmstate->cstate);
+	fmstate->cstate = NULL;
+	finish_foreign_modify(fmstate);
+}
+
+/*
+ *
+ * postgresExecForeignCopy
+ *		Send a number of tuples to the foreign relation.
+ */
+static void
+postgresExecForeignCopy(ResultRelInfo *resultRelInfo,
+						  TupleTableSlot **slots, int nslots)
+{
+	PgFdwModifyState *fmstate = resultRelInfo->ri_FdwState;
+	PGresult *res;
+	PGconn *conn = fmstate->conn;
+	bool OK = false;
+	int i;
+
+	/* Check correct use of CopyIn FDW API. */
+	Assert(fmstate->cstate != NULL);
+	Assert(copy_fmstate == NULL);
+
+	res = PQexec(conn, fmstate->query);
+	if (PQresultStatus(res) != PGRES_COPY_IN)
+		pgfdw_report_error(ERROR, res, conn, true, fmstate->query);
+	PQclear(res);
+
+	PG_TRY();
+	{
+		copy_fmstate = fmstate;
+		for (i = 0; i < nslots; i++)
+			CopyOneRowTo(fmstate->cstate, slots[i]);
+
+		OK = true;
+	}
+	PG_FINALLY();
+	{
+		copy_fmstate = NULL; /* Detect problems */
+
+		/* Finish COPY IN protocol. It is needed to do after successful copy or
+		 * after an error.
+		 */
+		if (PQputCopyEnd(conn, OK ? NULL : _("canceled by server")) <= 0 ||
+			PQflush(conn))
+			pgfdw_report_error(ERROR, NULL, fmstate->conn, false, fmstate->query);
+
+		/* After successfully  sending an EOF signal, check command OK. */
+		res = PQgetResult(conn);
+		if ((!OK && PQresultStatus(res) != PGRES_FATAL_ERROR) ||
+			(OK && PQresultStatus(res) != PGRES_COMMAND_OK))
+			pgfdw_report_error(ERROR, res, fmstate->conn, true, fmstate->query);
+
+		PQclear(res);
+		/* Do this to ensure we've pumped libpq back to idle state */
+		if (PQgetResult(conn) != NULL)
+			ereport(ERROR,
+					(errmsg("unexpected extra results during COPY of table: %s",
+							PQerrorMessage(conn))));
+
+		if (!OK)
+			PG_RE_THROW();
+	}
+	PG_END_TRY();
+}
+
 /*
  * postgresIsForeignRelUpdatable
  *		Determine whether a foreign table supports INSERT, UPDATE and/or
diff --git a/contrib/postgres_fdw/postgres_fdw.h b/contrib/postgres_fdw/postgres_fdw.h
index eef410db39..8fc5ff018f 100644
--- a/contrib/postgres_fdw/postgres_fdw.h
+++ b/contrib/postgres_fdw/postgres_fdw.h
@@ -162,6 +162,7 @@ extern void deparseInsertSql(StringInfo buf, RangeTblEntry *rte,
 							 List *targetAttrs, bool doNothing,
 							 List *withCheckOptionList, List *returningList,
 							 List **retrieved_attrs);
+extern void deparseCopyFromSql(StringInfo buf, Relation rel);
 extern void deparseUpdateSql(StringInfo buf, RangeTblEntry *rte,
 							 Index rtindex, Relation rel,
 							 List *targetAttrs,
diff --git a/contrib/postgres_fdw/sql/postgres_fdw.sql b/contrib/postgres_fdw/sql/postgres_fdw.sql
index 7581c5417b..22dcd12f02 100644
--- a/contrib/postgres_fdw/sql/postgres_fdw.sql
+++ b/contrib/postgres_fdw/sql/postgres_fdw.sql
@@ -2212,6 +2212,23 @@ alter table loc2 drop constraint loc2_f1positive;
 
 delete from rem2;
 
+create table foo (a int) partition by list (a);
+create table foo1 (like foo);
+create foreign table ffoo1 partition of foo for values in (1)
+	server loopback options (table_name 'foo1');
+create table foo2 (like foo);
+create foreign table ffoo2 partition of foo for values in (2)
+	server loopback options (table_name 'foo2');
+create function print_new_row() returns trigger language plpgsql as $$
+	begin raise notice '%', new; return new; end; $$;
+create trigger ffoo1_br_trig before insert on ffoo1
+	for each row execute function print_new_row();
+
+copy foo from stdin;
+1
+2
+\.
+
 -- Test local triggers
 create trigger trig_stmt_before before insert on rem2
 	for each statement execute procedure trigger_func();
@@ -2312,6 +2329,34 @@ drop trigger loc2_trig_row_before_insert on loc2;
 
 delete from rem2;
 
+alter table loc2 drop column f1;
+alter table loc2 drop column f2;
+copy rem2 from stdin;
+1	foo
+2	bar
+\.
+
+alter table loc2 add column f1 int;
+alter table loc2 add column f2 int;
+select * from rem2;
+
+-- dropped columns locally and on the foreign server
+alter table rem2 drop column f1;
+alter table rem2 drop column f2;
+copy rem2 from stdin;
+
+
+\.
+select * from rem2;
+
+alter table loc2 drop column f1;
+alter table loc2 drop column f2;
+copy rem2 from stdin;
+
+
+\.
+select * from rem2;
+
 -- test COPY FROM with foreign table created in the same transaction
 create table loc3 (f1 int, f2 text);
 begin;
diff --git a/doc/src/sgml/fdwhandler.sgml b/doc/src/sgml/fdwhandler.sgml
index 9c9293414c..a9a7402440 100644
--- a/doc/src/sgml/fdwhandler.sgml
+++ b/doc/src/sgml/fdwhandler.sgml
@@ -796,6 +796,79 @@ EndForeignInsert(EState *estate,
 
     <para>
 <programlisting>
+void
+BeginForeignCopy(ModifyTableState *mtstate,
+                   ResultRelInfo *rinfo);
+</programlisting>
+
+     Begin executing an copy operation on a foreign table. This routine is
+     called right before the first call of <function>ExecForeignCopy</function>
+     routine for the foreign table. It should perform any initialization needed
+     prior to the actual COPY FROM operation.
+     Subsequently, <function>ExecForeignCopy</function> will be called for
+     a bulk of tuples to be copied into the foreign table.
+    </para>
+
+    <para>
+     <literal>mtstate</literal> is the overall state of the
+     <structname>ModifyTable</structname> plan node being executed; global data about
+     the plan and execution state is available via this structure.
+     <literal>rinfo</literal> is the <structname>ResultRelInfo</structname> struct describing
+     the target foreign table.  (The <structfield>ri_FdwState</structfield> field of
+     <structname>ResultRelInfo</structname> is available for the FDW to store any
+     private state it needs for this operation.)
+    </para>
+
+    <para>
+     When this is called by a <command>COPY FROM</command> command, the
+     plan-related global data in <literal>mtstate</literal> is not provided.
+    </para>
+
+    <para>
+     If the <function>BeginForeignCopy</function> pointer is set to
+     <literal>NULL</literal>, no action is taken for the initialization.
+    </para>
+
+    <para>
+<programlisting>
+void
+EndForeignCopy(EState *estate,
+                 ResultRelInfo *rinfo);
+</programlisting>
+
+     End the copy operation and release resources.  It is normally not important
+     to release palloc'd memory, but for example open files and connections
+     to remote servers should be cleaned up.
+    </para>
+
+    <para>
+     If the <function>EndForeignCopy</function> pointer is set to
+     <literal>NULL</literal>, no action is taken for the termination.
+    </para>
+
+    <para>
+<programlisting>
+void
+ExecForeignCopy(ResultRelInfo *rinfo,
+                  TupleTableSlot **slots,
+                  int nslots);
+</programlisting>
+
+     Copy a bulk of tuples into the foreign table.
+      <literal>rinfo</literal> is the <structname>ResultRelInfo</structname> struct describing
+     the target foreign table.
+     <literal>slots</literal> contains the tuples to be inserted; it will match the
+     row-type definition of the foreign table.
+     <literal>nslots</literal> is a number of tuples in the <literal>slots</literal>
+    </para>
+
+    <para>
+     If the <function>ExecForeignCopy</function> pointer is set to
+     <literal>NULL</literal>, the <function>ExecForeignInsert</function> routine will be used to run COPY on the foreign table.
+    </para>
+
+    <para>
+<programlisting>
 int
 IsForeignRelUpdatable(Relation rel);
 </programlisting>
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 83ce196a45..26d79ad051 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -118,11 +118,14 @@ typedef struct CopyStateData
 
 	/* parameters from the COPY command */
 	Relation	rel;			/* relation to copy to or from */
+	TupleDesc	tupDesc;		/* COPY TO will be used for manual tuple copying
+								  * into the destination */
 	QueryDesc  *queryDesc;		/* executable query to copy from */
 	List	   *attnumlist;		/* integer list of attnums to copy */
 	char	   *filename;		/* filename, or NULL for STDIN/STDOUT */
 	bool		is_program;		/* is 'filename' a program to popen? */
 	copy_data_source_cb data_source_cb; /* function for reading data */
+	copy_data_dest_cb data_dest_cb;	/* function for writing data */
 	bool		binary;			/* binary format? */
 	bool		freeze;			/* freeze rows on loading? */
 	bool		csv_mode;		/* Comma Separated Value format? */
@@ -349,17 +352,12 @@ static const char BinarySignature[11] = "PGCOPY\n\377\r\n\0";
 
 /* non-export function prototypes */
 static CopyState BeginCopy(ParseState *pstate, bool is_from, Relation rel,
-						   RawStmt *raw_query, Oid queryRelId, List *attnamelist,
-						   List *options);
+						   TupleDesc srcTupDesc, RawStmt *raw_query,
+						   Oid queryRelId, List *attnamelist, List *options);
 static void EndCopy(CopyState cstate);
 static void ClosePipeToProgram(CopyState cstate);
-static CopyState BeginCopyTo(ParseState *pstate, Relation rel, RawStmt *query,
-							 Oid queryRelId, const char *filename, bool is_program,
-							 List *attnamelist, List *options);
-static void EndCopyTo(CopyState cstate);
 static uint64 DoCopyTo(CopyState cstate);
 static uint64 CopyTo(CopyState cstate);
-static void CopyOneRowTo(CopyState cstate, TupleTableSlot *slot);
 static bool CopyReadLine(CopyState cstate);
 static bool CopyReadLineText(CopyState cstate);
 static int	CopyReadAttributesText(CopyState cstate);
@@ -585,7 +583,13 @@ CopySendEndOfRow(CopyState cstate)
 			(void) pq_putmessage('d', fe_msgbuf->data, fe_msgbuf->len);
 			break;
 		case COPY_CALLBACK:
-			Assert(false);		/* Not yet supported. */
+			Assert(!cstate->binary);
+#ifndef WIN32
+			CopySendChar(cstate, '\n');
+#else
+			CopySendString(cstate, "\r\n");
+#endif
+			cstate->data_dest_cb(fe_msgbuf->data, fe_msgbuf->len);
 			break;
 	}
 
@@ -1114,8 +1118,8 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	}
 	else
 	{
-		cstate = BeginCopyTo(pstate, rel, query, relid,
-							 stmt->filename, stmt->is_program,
+		cstate = BeginCopyTo(pstate, rel, NULL, query, relid,
+							 stmt->filename, stmt->is_program, NULL,
 							 stmt->attlist, stmt->options);
 		*processed = DoCopyTo(cstate);	/* copy from database to file */
 		EndCopyTo(cstate);
@@ -1501,6 +1505,7 @@ static CopyState
 BeginCopy(ParseState *pstate,
 		  bool is_from,
 		  Relation rel,
+		  TupleDesc srcTupDesc,
 		  RawStmt *raw_query,
 		  Oid queryRelId,
 		  List *attnamelist,
@@ -1536,6 +1541,11 @@ BeginCopy(ParseState *pstate,
 
 		tupDesc = RelationGetDescr(cstate->rel);
 	}
+	else if (srcTupDesc)
+	{
+		Assert(!raw_query && !is_from);
+		tupDesc = cstate->tupDesc = srcTupDesc;
+	}
 	else
 	{
 		List	   *rewritten;
@@ -1862,20 +1872,25 @@ EndCopy(CopyState cstate)
 /*
  * Setup CopyState to read tuples from a table or a query for COPY TO.
  */
-static CopyState
+CopyState
 BeginCopyTo(ParseState *pstate,
 			Relation rel,
+			TupleDesc tupDesc,
 			RawStmt *query,
 			Oid queryRelId,
 			const char *filename,
 			bool is_program,
+			copy_data_dest_cb data_dest_cb,
 			List *attnamelist,
 			List *options)
 {
 	CopyState	cstate;
-	bool		pipe = (filename == NULL);
+	bool		pipe = (filename == NULL) && (data_dest_cb == NULL);
 	MemoryContext oldcontext;
 
+	/* Impossible to mix CopyTo modes */
+	Assert(rel == NULL || tupDesc == NULL);
+
 	if (rel != NULL && rel->rd_rel->relkind != RELKIND_RELATION)
 	{
 		if (rel->rd_rel->relkind == RELKIND_VIEW)
@@ -1914,8 +1929,9 @@ BeginCopyTo(ParseState *pstate,
 							RelationGetRelationName(rel))));
 	}
 
-	cstate = BeginCopy(pstate, false, rel, query, queryRelId, attnamelist,
-					   options);
+	cstate = BeginCopy(pstate, false, rel, tupDesc, query, queryRelId,
+					   attnamelist, options);
+
 	oldcontext = MemoryContextSwitchTo(cstate->copycontext);
 
 	if (pipe)
@@ -1924,6 +1940,11 @@ BeginCopyTo(ParseState *pstate,
 		if (whereToSendOutput != DestRemote)
 			cstate->copy_file = stdout;
 	}
+	else if (data_dest_cb)
+	{
+		cstate->copy_dest = COPY_CALLBACK;
+		cstate->data_dest_cb = data_dest_cb;
+	}
 	else
 	{
 		cstate->filename = pstrdup(filename);
@@ -2001,7 +2022,7 @@ BeginCopyTo(ParseState *pstate,
 static uint64
 DoCopyTo(CopyState cstate)
 {
-	bool		pipe = (cstate->filename == NULL);
+	bool		pipe = (cstate->filename == NULL) && (cstate->data_dest_cb == NULL);
 	bool		fe_copy = (pipe && whereToSendOutput == DestRemote);
 	uint64		processed;
 
@@ -2010,7 +2031,9 @@ DoCopyTo(CopyState cstate)
 		if (fe_copy)
 			SendCopyBegin(cstate);
 
+		CopyToStart(cstate);
 		processed = CopyTo(cstate);
+		CopyToFinish(cstate);
 
 		if (fe_copy)
 			SendCopyEnd(cstate);
@@ -2033,7 +2056,7 @@ DoCopyTo(CopyState cstate)
 /*
  * Clean up storage and release resources for COPY TO.
  */
-static void
+void
 EndCopyTo(CopyState cstate)
 {
 	if (cstate->queryDesc != NULL)
@@ -2049,19 +2072,22 @@ EndCopyTo(CopyState cstate)
 	EndCopy(cstate);
 }
 
-/*
- * Copy from relation or query TO file.
+/* Start COPY TO operation.
+ * Separated to the routine to prevent duplicate operations in the case of
+ * manual mode, where tuples are copied to the destination one by one, by call of
+ * the CopyOneRowTo() routine.
  */
-static uint64
-CopyTo(CopyState cstate)
+void
+CopyToStart(CopyState cstate)
 {
 	TupleDesc	tupDesc;
 	int			num_phys_attrs;
 	ListCell   *cur;
-	uint64		processed;
 
 	if (cstate->rel)
 		tupDesc = RelationGetDescr(cstate->rel);
+	else if (cstate->tupDesc)
+		tupDesc = cstate->tupDesc;
 	else
 		tupDesc = cstate->queryDesc->tupDesc;
 	num_phys_attrs = tupDesc->natts;
@@ -2148,6 +2174,32 @@ CopyTo(CopyState cstate)
 			CopySendEndOfRow(cstate);
 		}
 	}
+}
+
+/*
+ * Finish COPY TO operation.
+ */
+void
+CopyToFinish(CopyState cstate)
+{
+	if (cstate->binary)
+	{
+		/* Generate trailer for a binary copy */
+		CopySendInt16(cstate, -1);
+		/* Need to flush out the trailer */
+		CopySendEndOfRow(cstate);
+	}
+
+	MemoryContextDelete(cstate->rowcontext);
+}
+
+/*
+ * Copy from relation or query TO file.
+ */
+static uint64
+CopyTo(CopyState cstate)
+{
+	uint64		processed;
 
 	if (cstate->rel)
 	{
@@ -2179,24 +2231,13 @@ CopyTo(CopyState cstate)
 		ExecutorRun(cstate->queryDesc, ForwardScanDirection, 0L, true);
 		processed = ((DR_copy *) cstate->queryDesc->dest)->processed;
 	}
-
-	if (cstate->binary)
-	{
-		/* Generate trailer for a binary copy */
-		CopySendInt16(cstate, -1);
-		/* Need to flush out the trailer */
-		CopySendEndOfRow(cstate);
-	}
-
-	MemoryContextDelete(cstate->rowcontext);
-
 	return processed;
 }
 
 /*
  * Emit one row during CopyTo().
  */
-static void
+void
 CopyOneRowTo(CopyState cstate, TupleTableSlot *slot)
 {
 	bool		need_delim = false;
@@ -2486,54 +2527,63 @@ CopyMultiInsertBufferFlush(CopyMultiInsertInfo *miinfo,
 	cstate->line_buf_valid = false;
 	save_cur_lineno = cstate->cur_lineno;
 
-	/*
-	 * table_multi_insert may leak memory, so switch to short-lived memory
-	 * context before calling it.
-	 */
-	oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
-	table_multi_insert(resultRelInfo->ri_RelationDesc,
-					   slots,
-					   nused,
-					   mycid,
-					   ti_options,
-					   buffer->bistate);
-	MemoryContextSwitchTo(oldcontext);
-
-	for (i = 0; i < nused; i++)
+	if (resultRelInfo->ri_RelationDesc->rd_rel->relkind == RELKIND_FOREIGN_TABLE)
+	{
+		/* Flush into foreign table or partition */
+		resultRelInfo->ri_FdwRoutine->ExecForeignCopy(resultRelInfo,
+														slots,
+														nused);
+	}
+	else
 	{
 		/*
-		 * If there are any indexes, update them for all the inserted tuples,
-		 * and run AFTER ROW INSERT triggers.
+		 * table_multi_insert may leak memory, so switch to short-lived memory
+		 * context before calling it.
 		 */
-		if (resultRelInfo->ri_NumIndices > 0)
-		{
-			List	   *recheckIndexes;
-
-			cstate->cur_lineno = buffer->linenos[i];
-			recheckIndexes =
-				ExecInsertIndexTuples(resultRelInfo,
-									  buffer->slots[i], estate, false, NULL,
-									  NIL);
-			ExecARInsertTriggers(estate, resultRelInfo,
-								 slots[i], recheckIndexes,
-								 cstate->transition_capture);
-			list_free(recheckIndexes);
-		}
+		oldcontext = MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));
+		table_multi_insert(resultRelInfo->ri_RelationDesc,
+						   slots,
+						   nused,
+						   mycid,
+						   ti_options,
+						   buffer->bistate);
+		MemoryContextSwitchTo(oldcontext);
 
-		/*
-		 * There's no indexes, but see if we need to run AFTER ROW INSERT
-		 * triggers anyway.
-		 */
-		else if (resultRelInfo->ri_TrigDesc != NULL &&
-				 (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
-				  resultRelInfo->ri_TrigDesc->trig_insert_new_table))
+		for (i = 0; i < nused; i++)
 		{
-			cstate->cur_lineno = buffer->linenos[i];
-			ExecARInsertTriggers(estate, resultRelInfo,
-								 slots[i], NIL, cstate->transition_capture);
-		}
+			/*
+			 * If there are any indexes, update them for all the inserted tuples,
+			 * and run AFTER ROW INSERT triggers.
+			 */
+			if (resultRelInfo->ri_NumIndices > 0)
+			{
+				List	   *recheckIndexes;
+
+				cstate->cur_lineno = buffer->linenos[i];
+				recheckIndexes =
+					ExecInsertIndexTuples(resultRelInfo, buffer->slots[i],
+										  estate, false, NULL, NIL);
+				ExecARInsertTriggers(estate, resultRelInfo,
+									 slots[i], recheckIndexes,
+									 cstate->transition_capture);
+				list_free(recheckIndexes);
+			}
 
-		ExecClearTuple(slots[i]);
+			/*
+			 * There's no indexes, but see if we need to run AFTER ROW INSERT
+			 * triggers anyway.
+			 */
+			else if (resultRelInfo->ri_TrigDesc != NULL &&
+					 (resultRelInfo->ri_TrigDesc->trig_insert_after_row ||
+					  resultRelInfo->ri_TrigDesc->trig_insert_new_table))
+			{
+				cstate->cur_lineno = buffer->linenos[i];
+				ExecARInsertTriggers(estate, resultRelInfo,
+									 slots[i], NIL, cstate->transition_capture);
+			}
+
+			ExecClearTuple(slots[i]);
+		}
 	}
 
 	/* Mark that all slots are free */
@@ -2838,8 +2888,11 @@ CopyFrom(CopyState cstate)
 	 * checked by calling ExecRelationAllowsMultiInsert().  It does not matter
 	 * whether partitions have any volatile default expressions as we use the
 	 * defaults from the target of the COPY command.
+	 * Also, the COPY command requires a non-zero input list of attributes.
+	 * Therefore, the length of the attribute list is checked here.
 	 */
 	if (!cstate->volatile_defexprs &&
+		list_length(cstate->attnumlist) > 0 &&
 		!contain_volatile_functions(cstate->whereClause))
 		target_resultRelInfo->ri_usesMultiInsert =
 					ExecRelationAllowsMultiInsert(target_resultRelInfo, NULL);
@@ -2863,10 +2916,18 @@ CopyFrom(CopyState cstate)
 	 * Init COPY into foreign table. Initialization of copying into foreign
 	 * partitions will be done later.
 	 */
-	if (resultRelInfo->ri_FdwRoutine != NULL &&
-		resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
-		resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
-														 resultRelInfo);
+	if (target_resultRelInfo->ri_FdwRoutine != NULL)
+	{
+		if (target_resultRelInfo->ri_usesMultiInsert)
+		{
+			Assert(target_resultRelInfo->ri_FdwRoutine->BeginForeignCopy != NULL);
+			target_resultRelInfo->ri_FdwRoutine->BeginForeignCopy(mtstate,
+																  resultRelInfo);
+		}
+		else if (target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
+			target_resultRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate,
+																	resultRelInfo);
+	}
 
 	/* Prepare to catch AFTER triggers. */
 	AfterTriggerBeginQuery();
@@ -3261,10 +3322,16 @@ CopyFrom(CopyState cstate)
 	ExecResetTupleTable(estate->es_tupleTable, false);
 
 	/* Allow the FDW to shut down */
-	if (target_resultRelInfo->ri_FdwRoutine != NULL &&
-		target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
-		target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate,
-															  target_resultRelInfo);
+	if (target_resultRelInfo->ri_FdwRoutine != NULL)
+	{
+		if (target_resultRelInfo->ri_usesMultiInsert &&
+			target_resultRelInfo->ri_FdwRoutine->EndForeignCopy != NULL)
+			target_resultRelInfo->ri_FdwRoutine->EndForeignCopy(estate,
+														target_resultRelInfo);
+		else if (target_resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
+			target_resultRelInfo->ri_FdwRoutine->EndForeignInsert(estate,
+														target_resultRelInfo);
+	}
 
 	/* Tear down the multi-insert buffer data */
 	CopyMultiInsertInfoCleanup(&multiInsertInfo);
@@ -3315,7 +3382,8 @@ BeginCopyFrom(ParseState *pstate,
 	MemoryContext oldcontext;
 	bool		volatile_defexprs;
 
-	cstate = BeginCopy(pstate, true, rel, NULL, InvalidOid, attnamelist, options);
+	cstate = BeginCopy(pstate, true, rel, NULL, NULL, InvalidOid, attnamelist,
+																	options);
 	oldcontext = MemoryContextSwitchTo(cstate->copycontext);
 
 	/* Initialize state variables */
diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c
index 0ad98ff0e7..9d465cebaf 100644
--- a/src/backend/executor/execMain.c
+++ b/src/backend/executor/execMain.c
@@ -1291,8 +1291,12 @@ ExecRelationAllowsMultiInsert(const ResultRelInfo *rri,
 		rri->ri_TrigDesc->trig_insert_new_table)
 		return false;
 
-	/* Foreign tables don't support multi-inserts. */
-	if (rri->ri_FdwRoutine != NULL)
+	if (rri->ri_FdwRoutine != NULL &&
+		rri->ri_FdwRoutine->ExecForeignCopy == NULL)
+		/*
+		 * Foreign tables don't support multi-inserts, unless their FDW
+		 * provides the necessary COPY interface.
+		 */
 		return false;
 
 	/* OK, caller can use multi-insert on this relation. */
diff --git a/src/backend/executor/execPartition.c b/src/backend/executor/execPartition.c
index 121484374f..fae21356c7 100644
--- a/src/backend/executor/execPartition.c
+++ b/src/backend/executor/execPartition.c
@@ -1001,9 +1001,16 @@ ExecInitRoutingInfo(ModifyTableState *mtstate,
 	 * If the partition is a foreign table, let the FDW init itself for
 	 * routing tuples to the partition.
 	 */
-	if (partRelInfo->ri_FdwRoutine != NULL &&
-		partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
-		partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo);
+	if (partRelInfo->ri_FdwRoutine != NULL)
+	{
+		if (partRelInfo->ri_usesMultiInsert)
+		{
+			Assert(partRelInfo->ri_FdwRoutine->BeginForeignCopy != NULL);
+			partRelInfo->ri_FdwRoutine->BeginForeignCopy(mtstate, partRelInfo);
+		}
+		else if (partRelInfo->ri_FdwRoutine->BeginForeignInsert != NULL)
+			partRelInfo->ri_FdwRoutine->BeginForeignInsert(mtstate, partRelInfo);
+	}
 
 	partRelInfo->ri_PartitionInfo = partrouteinfo;
 	partRelInfo->ri_CopyMultiInsertBuffer = NULL;
@@ -1205,10 +1212,18 @@ ExecCleanupTupleRouting(ModifyTableState *mtstate,
 		ResultRelInfo *resultRelInfo = proute->partitions[i];
 
 		/* Allow any FDWs to shut down */
-		if (resultRelInfo->ri_FdwRoutine != NULL &&
-			resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
-			resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
-														   resultRelInfo);
+		if (resultRelInfo->ri_FdwRoutine != NULL)
+		{
+			if (resultRelInfo->ri_usesMultiInsert)
+			{
+				Assert(resultRelInfo->ri_FdwRoutine->EndForeignCopy != NULL);
+				resultRelInfo->ri_FdwRoutine->EndForeignCopy(mtstate->ps.state,
+															   resultRelInfo);
+			}
+			else if (resultRelInfo->ri_FdwRoutine->EndForeignInsert != NULL)
+				resultRelInfo->ri_FdwRoutine->EndForeignInsert(mtstate->ps.state,
+															   resultRelInfo);
+		}
 
 		/*
 		 * Check if this result rel is one belonging to the node's subplans,
diff --git a/src/include/commands/copy.h b/src/include/commands/copy.h
index c639833565..08309149ea 100644
--- a/src/include/commands/copy.h
+++ b/src/include/commands/copy.h
@@ -22,6 +22,7 @@
 /* CopyStateData is private in commands/copy.c */
 typedef struct CopyStateData *CopyState;
 typedef int (*copy_data_source_cb) (void *outbuf, int minread, int maxread);
+typedef void (*copy_data_dest_cb) (void *outbuf, int len);
 
 extern void DoCopy(ParseState *state, const CopyStmt *stmt,
 				   int stmt_location, int stmt_len,
@@ -39,6 +40,16 @@ extern void CopyFromErrorCallback(void *arg);
 
 extern uint64 CopyFrom(CopyState cstate);
 
+extern CopyState BeginCopyTo(ParseState *pstate, Relation rel,
+							 TupleDesc tupDesc, RawStmt *query,
+							 Oid queryRelId, const char *filename, bool is_program,
+							 copy_data_dest_cb data_dest_cb, List *attnamelist,
+							 List *options);
+extern void EndCopyTo(CopyState cstate);
+extern void CopyOneRowTo(CopyState cstate, TupleTableSlot *slot);
+extern void CopyToStart(CopyState cstate);
+extern void CopyToFinish(CopyState cstate);
+
 extern DestReceiver *CreateCopyDestReceiver(void);
 
 #endif							/* COPY_H */
diff --git a/src/include/foreign/fdwapi.h b/src/include/foreign/fdwapi.h
index 95556dfb15..52b213f5aa 100644
--- a/src/include/foreign/fdwapi.h
+++ b/src/include/foreign/fdwapi.h
@@ -104,6 +104,16 @@ typedef void (*BeginForeignInsert_function) (ModifyTableState *mtstate,
 typedef void (*EndForeignInsert_function) (EState *estate,
 										   ResultRelInfo *rinfo);
 
+typedef void (*BeginForeignCopy_function) (ModifyTableState *mtstate,
+										   ResultRelInfo *rinfo);
+
+typedef void (*ExecForeignCopy_function) (ResultRelInfo *rinfo,
+										  TupleTableSlot **slots,
+										  int nslots);
+
+typedef void (*EndForeignCopy_function) (EState *estate,
+										 ResultRelInfo *rinfo);
+
 typedef int (*IsForeignRelUpdatable_function) (Relation rel);
 
 typedef bool (*PlanDirectModify_function) (PlannerInfo *root,
@@ -220,6 +230,11 @@ typedef struct FdwRoutine
 	IterateDirectModify_function IterateDirectModify;
 	EndDirectModify_function EndDirectModify;
 
+	/* Support functions for COPY into foreign tables */
+	BeginForeignCopy_function BeginForeignCopy;
+	ExecForeignCopy_function ExecForeignCopy;
+	EndForeignCopy_function EndForeignCopy;
+
 	/* Functions for SELECT FOR UPDATE/SHARE row locking */
 	GetForeignRowMarkType_function GetForeignRowMarkType;
 	RefetchForeignRow_function RefetchForeignRow;
-- 
2.17.1


--------------545C3421673CBF7E5637A5E1
Content-Type: text/x-patch; charset=UTF-8;
 name="v11-0001-Move-multi-insert-decision-logic-into-executor.patch"
Content-Transfer-Encoding: 7bit
Content-Disposition: attachment;
 filename*0="v11-0001-Move-multi-insert-decision-logic-into-executor.patc";
 filename*1="h"



^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 15:34  Tom Lane <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: Tom Lane @ 2023-01-27 15:34 UTC (permalink / raw)
  To: Peter Eisentraut <[email protected]>; +Cc: [email protected]

Peter Eisentraut <[email protected]> writes:
> On 25.01.23 22:25, Tom Lane wrote:
>> The specified settings
>> are applied on the command line of the initial probe calls
>> (which happen before we've made any config files), and then they
>> are added to postgresql.auto.conf, which causes them to take
>> effect for the bootstrap backend runs as well as subsequent
>> postmaster starts.

> I would have expected them to be edited into postgresql.conf.  What are 
> the arguments for one or the other?

TBH, the driving reason was that the string-munging code we have in
initdb isn't up to snuff for that: it wants to substitute for an
exactly-known string, which we won't have in the case of an
arbitrary GUC.

One idea if we want to make it work like that could be to stop
trying to edit out the default value, and instead make the file
contents look like, say,

#huge_pages = try                   # on, off, or try
huge_pages = off                    # set by initdb

Then we just need to be able to find the GUC's entry.

> Btw., something that I have had in my notes for a while, but with this 
> it would now be officially exposed:  Not all options can be safely set 
> during bootstrap.  For example,
>      initdb -D data -c track_commit_timestamp=on
> will fail an assertion.  This might be an exception, or there might be 
> others.

Interesting.  We'd probably want to sprinkle some more
do-nothing-in-bootstrap-mode tests as we discover that sort of thing.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 15:41  Robert Haas <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: Robert Haas @ 2023-01-27 15:41 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; [email protected]

On Fri, Jan 27, 2023 at 10:34 AM Tom Lane <[email protected]> wrote:
> One idea if we want to make it work like that could be to stop
> trying to edit out the default value, and instead make the file
> contents look like, say,
>
> #huge_pages = try                   # on, off, or try
> huge_pages = off                    # set by initdb

How about just making replace_token() a little smarter, and maybe renaming it?

The idea is that instead of:

replace_token(conflines, "#max_connections = 100", repltok);

You'd write something like:

replace_guc_value(conflines, "max_connections", repltok);

Which would look for a line matching /^#max_connections\s+=\s/, and
then identify everything following that point up to the first #. It
would replace all that stuff with repltok, but if the replacement is
shorter than the original, it would pad with spaces to get back to the
original length. And otherwise it would add a single space, so that if
you set a super long GUC value there's still at least one space
between the end of the value and the comment that follows.

There might be some quoting-related problems with this idea, not sure.

-- 
Robert Haas
EDB: http://www.enterprisedb.com






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 15:53  Tom Lane <[email protected]>
  parent: Robert Haas <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: Tom Lane @ 2023-01-27 15:53 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Peter Eisentraut <[email protected]>; [email protected]

Robert Haas <[email protected]> writes:
> The idea is that instead of:

> replace_token(conflines, "#max_connections = 100", repltok);

> You'd write something like:

> replace_guc_value(conflines, "max_connections", repltok);

> Which would look for a line matching /^#max_connections\s+=\s/, and
> then identify everything following that point up to the first #. It
> would replace all that stuff with repltok, but if the replacement is
> shorter than the original, it would pad with spaces to get back to the
> original length. And otherwise it would add a single space, so that if
> you set a super long GUC value there's still at least one space
> between the end of the value and the comment that follows.

Well, yeah, I was trying to avoid writing that ;-).  There's even
one more wrinkle: we might already have removed the initial '#',
if one does say "-c max_connections=N", because this logic won't
know whether the -c switch matches one of initdb's predetermined
substitutions.

> There might be some quoting-related problems with this idea, not sure.

'#' in a value might confuse it, but we could probably take the last '#'
not the first.

Anyway, it seems like I gotta work harder.  I'll produce a
new patch.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 16:01  David G. Johnston <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: David G. Johnston @ 2023-01-27 16:01 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

On Fri, Jan 27, 2023 at 8:53 AM Tom Lane <[email protected]> wrote:

> Robert Haas <[email protected]> writes:
> > The idea is that instead of:
>
> > replace_token(conflines, "#max_connections = 100", repltok);
>
> > You'd write something like:
>
> > replace_guc_value(conflines, "max_connections", repltok);
>
> > Which would look for a line matching /^#max_connections\s+=\s/, and
> > then identify everything following that point up to the first #. It
> > would replace all that stuff with repltok, but if the replacement is
> > shorter than the original, it would pad with spaces to get back to the
> > original length. And otherwise it would add a single space, so that if
> > you set a super long GUC value there's still at least one space
> > between the end of the value and the comment that follows.
>
> Well, yeah, I was trying to avoid writing that ;-).  There's even
> one more wrinkle: we might already have removed the initial '#',
> if one does say "-c max_connections=N", because this logic won't
> know whether the -c switch matches one of initdb's predetermined
> substitutions.
>
> > There might be some quoting-related problems with this idea, not sure.
>
> '#' in a value might confuse it, but we could probably take the last '#'
> not the first.
>
> Anyway, it seems like I gotta work harder.  I'll produce a
> new patch.
>
>
How about just adding a "section" to the end of the file as needed:

# AdHoc Settings Specified During InitDB
max_connections=75
...

David J.


^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 16:35  Tom Lane <[email protected]>
  parent: David G. Johnston <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: Tom Lane @ 2023-01-27 16:35 UTC (permalink / raw)
  To: David G. Johnston <[email protected]>; +Cc: Robert Haas <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

"David G. Johnston" <[email protected]> writes:
> On Fri, Jan 27, 2023 at 8:53 AM Tom Lane <[email protected]> wrote:
>> Anyway, it seems like I gotta work harder.  I'll produce a
>> new patch.

> How about just adding a "section" to the end of the file as needed:

> # AdHoc Settings Specified During InitDB
> max_connections=75
> ...

Nah, I think that would be impossibly confusing.  One way or another
the live setting has to be near where the GUC is documented.

We will have to do add-at-the-end for custom GUCs, of course,
but in that case there's no matching comment to confuse you.

			regards, tom lane






^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-01-27 20:02  Tom Lane <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 1 reply; 8+ messages in thread

From: Tom Lane @ 2023-01-27 20:02 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: David G. Johnston <[email protected]>; Peter Eisentraut <[email protected]>; [email protected]

I wrote:
>>> Anyway, it seems like I gotta work harder.  I'll produce a
>>> new patch.

The string-hacking was fully as tedious as I expected.  However, the
output looks pretty nice, and this does have the advantage that the
pre-programmed substitutions become a lot more robust: they are no
longer dependent on the initdb code exactly matching what is in
postgresql.conf.sample.

			regards, tom lane



Attachments:

  [text/x-diff] set-arbitrary-options-during-initdb-2.patch (22.8K, ../../[email protected]/2-set-arbitrary-options-during-initdb-2.patch)
  download | inline diff:
diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index 5b2bdac101..724188b1b5 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -433,6 +433,23 @@ PostgreSQL documentation
     Other, less commonly used, options are also available:
 
     <variablelist>
+     <varlistentry id="app-initdb-option-set">
+      <term><option>-c <replaceable>name</replaceable>=<replaceable>value</replaceable></option></term>
+      <term><option>--set <replaceable>name</replaceable>=<replaceable>value</replaceable></option></term>
+      <listitem>
+       <para>
+        Forcibly set the server parameter <replaceable>name</replaceable>
+        to <replaceable>value</replaceable> during <command>initdb</command>,
+        and also install that setting in the
+        generated <filename>postgresql.conf</filename> file,
+        so that it will apply during future server runs.
+        This option can be given more than once to set several parameters.
+        It is primarily useful when the environment is such that the server
+        will not start at all using the default parameters.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="app-initdb-option-debug">
       <term><option>-d</option></term>
       <term><option>--debug</option></term>
diff --git a/src/bin/initdb/initdb.c b/src/bin/initdb/initdb.c
index 7a58c33ace..c955c0837e 100644
--- a/src/bin/initdb/initdb.c
+++ b/src/bin/initdb/initdb.c
@@ -48,6 +48,7 @@
 
 #include "postgres_fe.h"
 
+#include <ctype.h>
 #include <dirent.h>
 #include <fcntl.h>
 #include <netdb.h>
@@ -82,6 +83,13 @@
 /* Ideally this would be in a .h file, but it hardly seems worth the trouble */
 extern const char *select_default_timezone(const char *share_path);
 
+/* simple list of strings */
+typedef struct _stringlist
+{
+	char	   *str;
+	struct _stringlist *next;
+} _stringlist;
+
 static const char *const auth_methods_host[] = {
 	"trust", "reject", "scram-sha-256", "md5", "password", "ident", "radius",
 #ifdef ENABLE_GSS
@@ -142,6 +150,8 @@ static char *pwfilename = NULL;
 static char *superuser_password = NULL;
 static const char *authmethodhost = NULL;
 static const char *authmethodlocal = NULL;
+static _stringlist *extra_guc_names = NULL;
+static _stringlist *extra_guc_values = NULL;
 static bool debug = false;
 static bool noclean = false;
 static bool noinstructions = false;
@@ -242,7 +252,10 @@ static char backend_exec[MAXPGPATH];
 
 static char **replace_token(char **lines,
 							const char *token, const char *replacement);
-
+static char **replace_guc_value(char **lines,
+								const char *guc_name, const char *guc_value,
+								bool mark_as_comment);
+static bool guc_value_requires_quotes(const char *guc_value);
 static char **readfile(const char *path);
 static void writefile(char *path, char **lines);
 static FILE *popen_check(const char *command, const char *mode);
@@ -253,6 +266,7 @@ static void check_input(char *path);
 static void write_version_file(const char *extrapath);
 static void set_null_conf(void);
 static void test_config_settings(void);
+static bool test_specific_config_settings(int test_conns, int test_buffs);
 static void setup_config(void);
 static void bootstrap_template1(void);
 static void setup_auth(FILE *cmdfd);
@@ -360,9 +374,34 @@ escape_quotes_bki(const char *src)
 }
 
 /*
- * make a copy of the array of lines, with token replaced by replacement
+ * Add an item at the end of a stringlist.
+ */
+static void
+add_stringlist_item(_stringlist **listhead, const char *str)
+{
+	_stringlist *newentry = pg_malloc(sizeof(_stringlist));
+	_stringlist *oldentry;
+
+	newentry->str = pg_strdup(str);
+	newentry->next = NULL;
+	if (*listhead == NULL)
+		*listhead = newentry;
+	else
+	{
+		for (oldentry = *listhead; oldentry->next; oldentry = oldentry->next)
+			 /* skip */ ;
+		oldentry->next = newentry;
+	}
+}
+
+/*
+ * Make a copy of the array of lines, with token replaced by replacement
  * the first time it occurs on each line.
  *
+ * The original data structure is not changed, but we share any unchanged
+ * strings with it.  (This definition lends itself to memory leaks, but
+ * we don't care too much about leaks in this program.)
+ *
  * This does most of what sed was used for in the shell script, but
  * doesn't need any regexp stuff.
  */
@@ -416,6 +455,168 @@ replace_token(char **lines, const char *token, const char *replacement)
 	return result;
 }
 
+/*
+ * Make a copy of the array of lines, replacing the possibly-commented-out
+ * assignment of parameter guc_name with a live assignment of guc_value.
+ * The value will be suitably quoted.
+ *
+ * If mark_as_comment is true, the replacement line is prefixed with '#'.
+ * This is used for fixing up cases where the effective default might not
+ * match what is in postgresql.conf.sample.
+ *
+ * We assume there's at most one matching assignment.  If we find no match,
+ * append a new line with the desired assignment.
+ *
+ * The original data structure is not changed, but we share any unchanged
+ * strings with it.  (This definition lends itself to memory leaks, but
+ * we don't care too much about leaks in this program.)
+ */
+static char **
+replace_guc_value(char **lines, const char *guc_name, const char *guc_value,
+				  bool mark_as_comment)
+{
+	char	  **result;
+	int			namelen = strlen(guc_name);
+	PQExpBuffer newline = createPQExpBuffer();
+	int			numlines = 0;
+	int			i;
+
+	/* prepare the replacement line, except for possible comment and newline */
+	if (mark_as_comment)
+		appendPQExpBufferChar(newline, '#');
+	appendPQExpBuffer(newline, "%s = ", guc_name);
+	if (guc_value_requires_quotes(guc_value))
+		appendPQExpBuffer(newline, "'%s'", escape_quotes(guc_value));
+	else
+		appendPQExpBufferStr(newline, guc_value);
+
+	/* create the new pointer array */
+	for (i = 0; lines[i]; i++)
+		numlines++;
+
+	/* leave room for one extra string in case we need to append */
+	result = (char **) pg_malloc((numlines + 2) * sizeof(char *));
+
+	/* initialize result with all the same strings */
+	memcpy(result, lines, (numlines + 1) * sizeof(char *));
+
+	for (i = 0; i < numlines; i++)
+	{
+		const char *where;
+
+		/*
+		 * Look for a line assigning to guc_name.  Typically it will be
+		 * preceded by '#', but that might not be the case if a -c switch
+		 * overrides a previous assignment.  We allow leading whitespace too,
+		 * although normally there wouldn't be any.
+		 */
+		where = result[i];
+		while (*where == '#' || isspace((unsigned char) *where))
+			where++;
+		if (strncmp(where, guc_name, namelen) != 0)
+			continue;
+		where += namelen;
+		while (isspace((unsigned char) *where))
+			where++;
+		if (*where != '=')
+			continue;
+
+		/* found it -- append the original comment if any */
+		where = strrchr(where, '#');
+		if (where)
+		{
+			/*
+			 * We try to preserve original indentation, which is tedious.
+			 * oldindent and newindent are measured in de-tab-ified columns.
+			 */
+			const char *ptr;
+			int			oldindent = 0;
+			int			newindent;
+
+			for (ptr = result[i]; ptr < where; ptr++)
+			{
+				if (*ptr == '\t')
+					oldindent += 8 - (oldindent % 8);
+				else
+					oldindent++;
+			}
+			/* ignore the possibility of tabs in guc_value */
+			newindent = newline->len;
+			/* append appropriate tabs and spaces, forcing at least one */
+			oldindent = Max(oldindent, newindent + 1);
+			while (newindent < oldindent)
+			{
+				int			newindent_if_tab = newindent + 8 - (newindent % 8);
+
+				if (newindent_if_tab <= oldindent)
+				{
+					appendPQExpBufferChar(newline, '\t');
+					newindent = newindent_if_tab;
+				}
+				else
+				{
+					appendPQExpBufferChar(newline, ' ');
+					newindent++;
+				}
+			}
+			/* and finally append the old comment */
+			appendPQExpBufferStr(newline, where);
+			/* we'll have appended the original newline; don't add another */
+		}
+		else
+			appendPQExpBufferChar(newline, '\n');
+
+		result[i] = newline->data;
+
+		break;					/* assume there's only one match */
+	}
+
+	if (i >= numlines)
+	{
+		/*
+		 * No match, so append a new entry.  (We rely on the bootstrap server
+		 * to complain if it's not a valid GUC name.)
+		 */
+		appendPQExpBufferChar(newline, '\n');
+		result[numlines++] = newline->data;
+		result[numlines] = NULL;	/* keep the array null-terminated */
+	}
+
+	return result;
+}
+
+/*
+ * Decide if we should quote a replacement GUC value.  We aren't too tense
+ * here, but we'd like to avoid quoting simple identifiers and numbers
+ * with units, which are common cases.
+ */
+static bool
+guc_value_requires_quotes(const char *guc_value)
+{
+	/* Don't use <ctype.h> macros here, they might accept too much */
+#define LETTERS	"abcdefghijklmnopqrstuvwxyzABCDEFGHIJKLMNOPQRSTUVWXYZ"
+#define DIGITS	"0123456789"
+
+	if (*guc_value == '\0')
+		return true;			/* empty string must be quoted */
+	if (strchr(LETTERS, *guc_value))
+	{
+		if (strspn(guc_value, LETTERS DIGITS) == strlen(guc_value))
+			return false;		/* it's an identifier */
+		return true;			/* nope */
+	}
+	if (strchr(DIGITS, *guc_value))
+	{
+		/* skip over digits */
+		guc_value += strspn(guc_value, DIGITS);
+		/* there can be zero or more unit letters after the digits */
+		if (strspn(guc_value, LETTERS) == strlen(guc_value))
+			return false;		/* it's a number, possibly with units */
+		return true;			/* nope */
+	}
+	return true;				/* all else must be quoted */
+}
+
 /*
  * get the lines from a text file
  */
@@ -873,11 +1074,9 @@ test_config_settings(void)
 		400, 300, 200, 100, 50
 	};
 
-	char		cmd[MAXPGPATH];
 	const int	connslen = sizeof(trial_conns) / sizeof(int);
 	const int	bufslen = sizeof(trial_bufs) / sizeof(int);
 	int			i,
-				status,
 				test_conns,
 				test_buffs,
 				ok_buffers = 0;
@@ -903,19 +1102,7 @@ test_config_settings(void)
 		test_conns = trial_conns[i];
 		test_buffs = MIN_BUFS_FOR_CONNS(test_conns);
 
-		snprintf(cmd, sizeof(cmd),
-				 "\"%s\" --check %s %s "
-				 "-c max_connections=%d "
-				 "-c shared_buffers=%d "
-				 "-c dynamic_shared_memory_type=%s "
-				 "< \"%s\" > \"%s\" 2>&1",
-				 backend_exec, boot_options, extra_options,
-				 test_conns, test_buffs,
-				 dynamic_shared_memory_type,
-				 DEVNULL, DEVNULL);
-		fflush(NULL);
-		status = system(cmd);
-		if (status == 0)
+		if (test_specific_config_settings(test_conns, test_buffs))
 		{
 			ok_buffers = test_buffs;
 			break;
@@ -940,19 +1127,7 @@ test_config_settings(void)
 			break;
 		}
 
-		snprintf(cmd, sizeof(cmd),
-				 "\"%s\" --check %s %s "
-				 "-c max_connections=%d "
-				 "-c shared_buffers=%d "
-				 "-c dynamic_shared_memory_type=%s "
-				 "< \"%s\" > \"%s\" 2>&1",
-				 backend_exec, boot_options, extra_options,
-				 n_connections, test_buffs,
-				 dynamic_shared_memory_type,
-				 DEVNULL, DEVNULL);
-		fflush(NULL);
-		status = system(cmd);
-		if (status == 0)
+		if (test_specific_config_settings(n_connections, test_buffs))
 			break;
 	}
 	n_buffers = test_buffs;
@@ -968,6 +1143,48 @@ test_config_settings(void)
 	printf("%s\n", default_timezone ? default_timezone : "GMT");
 }
 
+/*
+ * Test a specific combination of configuration settings.
+ */
+static bool
+test_specific_config_settings(int test_conns, int test_buffs)
+{
+	PQExpBuffer cmd = createPQExpBuffer();
+	_stringlist *gnames,
+			   *gvalues;
+	int			status;
+
+	/* Set up the test postmaster invocation */
+	printfPQExpBuffer(cmd,
+					  "\"%s\" --check %s %s "
+					  "-c max_connections=%d "
+					  "-c shared_buffers=%d "
+					  "-c dynamic_shared_memory_type=%s",
+					  backend_exec, boot_options, extra_options,
+					  test_conns, test_buffs,
+					  dynamic_shared_memory_type);
+
+	/* Add any user-given setting overrides */
+	for (gnames = extra_guc_names, gvalues = extra_guc_values;
+		 gnames != NULL;		/* assume lists have the same length */
+		 gnames = gnames->next, gvalues = gvalues->next)
+	{
+		appendPQExpBuffer(cmd, " -c %s=", gnames->str);
+		appendShellString(cmd, gvalues->str);
+	}
+
+	appendPQExpBuffer(cmd,
+					  " < \"%s\" > \"%s\" 2>&1",
+					  DEVNULL, DEVNULL);
+
+	fflush(NULL);
+	status = system(cmd->data);
+
+	destroyPQExpBuffer(cmd);
+
+	return (status == 0);
+}
+
 /*
  * Calculate the default wal_size with a "pretty" unit.
  */
@@ -995,6 +1212,8 @@ setup_config(void)
 	char		repltok[MAXPGPATH];
 	char		path[MAXPGPATH];
 	char	   *autoconflines[3];
+	_stringlist *gnames,
+			   *gvalues;
 
 	fputs(_("creating configuration files ... "), stdout);
 	fflush(stdout);
@@ -1003,120 +1222,106 @@ setup_config(void)
 
 	conflines = readfile(conf_file);
 
-	snprintf(repltok, sizeof(repltok), "max_connections = %d", n_connections);
-	conflines = replace_token(conflines, "#max_connections = 100", repltok);
+	snprintf(repltok, sizeof(repltok), "%d", n_connections);
+	conflines = replace_guc_value(conflines, "max_connections",
+								  repltok, false);
 
 	if ((n_buffers * (BLCKSZ / 1024)) % 1024 == 0)
-		snprintf(repltok, sizeof(repltok), "shared_buffers = %dMB",
+		snprintf(repltok, sizeof(repltok), "%dMB",
 				 (n_buffers * (BLCKSZ / 1024)) / 1024);
 	else
-		snprintf(repltok, sizeof(repltok), "shared_buffers = %dkB",
+		snprintf(repltok, sizeof(repltok), "%dkB",
 				 n_buffers * (BLCKSZ / 1024));
-	conflines = replace_token(conflines, "#shared_buffers = 128MB", repltok);
+	conflines = replace_guc_value(conflines, "shared_buffers",
+								  repltok, false);
 
-	snprintf(repltok, sizeof(repltok), "#unix_socket_directories = '%s'",
-			 DEFAULT_PGSOCKET_DIR);
-	conflines = replace_token(conflines, "#unix_socket_directories = '/tmp'",
-							  repltok);
+	conflines = replace_guc_value(conflines, "lc_messages", lc_messages, false);
 
-#if DEF_PGPORT != 5432
-	snprintf(repltok, sizeof(repltok), "#port = %d", DEF_PGPORT);
-	conflines = replace_token(conflines, "#port = 5432", repltok);
-#endif
+	conflines = replace_guc_value(conflines, "lc_monetary", lc_monetary, false);
 
-	/* set default max_wal_size and min_wal_size */
-	snprintf(repltok, sizeof(repltok), "min_wal_size = %s",
-			 pretty_wal_size(DEFAULT_MIN_WAL_SEGS));
-	conflines = replace_token(conflines, "#min_wal_size = 80MB", repltok);
+	conflines = replace_guc_value(conflines, "lc_numeric", lc_numeric, false);
 
-	snprintf(repltok, sizeof(repltok), "max_wal_size = %s",
-			 pretty_wal_size(DEFAULT_MAX_WAL_SEGS));
-	conflines = replace_token(conflines, "#max_wal_size = 1GB", repltok);
-
-	snprintf(repltok, sizeof(repltok), "lc_messages = '%s'",
-			 escape_quotes(lc_messages));
-	conflines = replace_token(conflines, "#lc_messages = 'C'", repltok);
-
-	snprintf(repltok, sizeof(repltok), "lc_monetary = '%s'",
-			 escape_quotes(lc_monetary));
-	conflines = replace_token(conflines, "#lc_monetary = 'C'", repltok);
-
-	snprintf(repltok, sizeof(repltok), "lc_numeric = '%s'",
-			 escape_quotes(lc_numeric));
-	conflines = replace_token(conflines, "#lc_numeric = 'C'", repltok);
-
-	snprintf(repltok, sizeof(repltok), "lc_time = '%s'",
-			 escape_quotes(lc_time));
-	conflines = replace_token(conflines, "#lc_time = 'C'", repltok);
+	conflines = replace_guc_value(conflines, "lc_time", lc_time, false);
 
 	switch (locale_date_order(lc_time))
 	{
 		case DATEORDER_YMD:
-			strcpy(repltok, "datestyle = 'iso, ymd'");
+			strcpy(repltok, "iso, ymd");
 			break;
 		case DATEORDER_DMY:
-			strcpy(repltok, "datestyle = 'iso, dmy'");
+			strcpy(repltok, "iso, dmy");
 			break;
 		case DATEORDER_MDY:
 		default:
-			strcpy(repltok, "datestyle = 'iso, mdy'");
+			strcpy(repltok, "iso, mdy");
 			break;
 	}
-	conflines = replace_token(conflines, "#datestyle = 'iso, mdy'", repltok);
+	conflines = replace_guc_value(conflines, "datestyle", repltok, false);
 
 	snprintf(repltok, sizeof(repltok),
-			 "default_text_search_config = 'pg_catalog.%s'",
-			 escape_quotes(default_text_search_config));
-	conflines = replace_token(conflines,
-							  "#default_text_search_config = 'pg_catalog.simple'",
-							  repltok);
+			 "pg_catalog.%s", default_text_search_config);
+	conflines = replace_guc_value(conflines, "default_text_search_config",
+								  repltok, false);
 
 	if (default_timezone)
 	{
-		snprintf(repltok, sizeof(repltok), "timezone = '%s'",
-				 escape_quotes(default_timezone));
-		conflines = replace_token(conflines, "#timezone = 'GMT'", repltok);
-		snprintf(repltok, sizeof(repltok), "log_timezone = '%s'",
-				 escape_quotes(default_timezone));
-		conflines = replace_token(conflines, "#log_timezone = 'GMT'", repltok);
+		conflines = replace_guc_value(conflines, "timezone",
+									  default_timezone, false);
+		conflines = replace_guc_value(conflines, "log_timezone",
+									  default_timezone, false);
 	}
 
-	snprintf(repltok, sizeof(repltok), "dynamic_shared_memory_type = %s",
-			 dynamic_shared_memory_type);
-	conflines = replace_token(conflines, "#dynamic_shared_memory_type = posix",
-							  repltok);
+	conflines = replace_guc_value(conflines, "dynamic_shared_memory_type",
+								  dynamic_shared_memory_type, false);
+
+	/*
+	 * Fix up various entries to match the true compile-time defaults.  Since
+	 * these are indeed defaults, keep the postgresql.conf lines commented.
+	 */
+	conflines = replace_guc_value(conflines, "unix_socket_directories",
+								  DEFAULT_PGSOCKET_DIR, true);
+
+#if DEF_PGPORT != 5432
+	snprintf(repltok, sizeof(repltok), "%d", DEF_PGPORT);
+	conflines = replace_guc_value(conflines, "port",
+								  repltok, true);
+#endif
+
+	conflines = replace_guc_value(conflines, "min_wal_size",
+								  pretty_wal_size(DEFAULT_MIN_WAL_SEGS), true);
+
+	conflines = replace_guc_value(conflines, "max_wal_size",
+								  pretty_wal_size(DEFAULT_MAX_WAL_SEGS), true);
 
 #if DEFAULT_BACKEND_FLUSH_AFTER > 0
-	snprintf(repltok, sizeof(repltok), "#backend_flush_after = %dkB",
+	snprintf(repltok, sizeof(repltok), "%dkB",
 			 DEFAULT_BACKEND_FLUSH_AFTER * (BLCKSZ / 1024));
-	conflines = replace_token(conflines, "#backend_flush_after = 0",
-							  repltok);
+	conflines = replace_guc_value(conflines, "backend_flush_after",
+								  repltok, true);
 #endif
 
 #if DEFAULT_BGWRITER_FLUSH_AFTER > 0
-	snprintf(repltok, sizeof(repltok), "#bgwriter_flush_after = %dkB",
+	snprintf(repltok, sizeof(repltok), "%dkB",
 			 DEFAULT_BGWRITER_FLUSH_AFTER * (BLCKSZ / 1024));
-	conflines = replace_token(conflines, "#bgwriter_flush_after = 0",
-							  repltok);
+	conflines = replace_guc_value(conflines, "bgwriter_flush_after",
+								  repltok, true);
 #endif
 
 #if DEFAULT_CHECKPOINT_FLUSH_AFTER > 0
-	snprintf(repltok, sizeof(repltok), "#checkpoint_flush_after = %dkB",
+	snprintf(repltok, sizeof(repltok), "%dkB",
 			 DEFAULT_CHECKPOINT_FLUSH_AFTER * (BLCKSZ / 1024));
-	conflines = replace_token(conflines, "#checkpoint_flush_after = 0",
-							  repltok);
+	conflines = replace_guc_value(conflines, "checkpoint_flush_after",
+								  repltok, true);
 #endif
 
 #ifndef USE_PREFETCH
-	conflines = replace_token(conflines,
-							  "#effective_io_concurrency = 1",
-							  "#effective_io_concurrency = 0");
+	conflines = replace_guc_value(conflines, "effective_io_concurrency",
+								  "0", true);
 #endif
 
 #ifdef WIN32
-	conflines = replace_token(conflines,
-							  "#update_process_title = on",
-							  "#update_process_title = off");
+	conflines = replace_guc_value(conflines, "update_process_title",
+								  "off", true);
 #endif
 
 	/*
@@ -1128,9 +1333,8 @@ setup_config(void)
 		(strcmp(authmethodhost, "md5") == 0 &&
 		 strcmp(authmethodlocal, "scram-sha-256") != 0))
 	{
-		conflines = replace_token(conflines,
-								  "#password_encryption = scram-sha-256",
-								  "password_encryption = md5");
+		conflines = replace_guc_value(conflines, "password_encryption",
+									  "md5", false);
 	}
 
 	/*
@@ -1141,11 +1345,22 @@ setup_config(void)
 	 */
 	if (pg_dir_create_mode == PG_DIR_MODE_GROUP)
 	{
-		conflines = replace_token(conflines,
-								  "#log_file_mode = 0600",
-								  "log_file_mode = 0640");
+		conflines = replace_guc_value(conflines, "log_file_mode",
+									  "0640", false);
 	}
 
+	/*
+	 * Now replace anything that's overridden via -c switches.
+	 */
+	for (gnames = extra_guc_names, gvalues = extra_guc_values;
+		 gnames != NULL;		/* assume lists have the same length */
+		 gnames = gnames->next, gvalues = gvalues->next)
+	{
+		conflines = replace_guc_value(conflines, gnames->str,
+									  gvalues->str, false);
+	}
+
+	/* ... and write out the finished postgresql.conf file */
 	snprintf(path, sizeof(path), "%s/postgresql.conf", pg_data);
 
 	writefile(path, conflines);
@@ -2124,6 +2339,7 @@ usage(const char *progname)
 	printf(_("  -X, --waldir=WALDIR       location for the write-ahead log directory\n"));
 	printf(_("      --wal-segsize=SIZE    size of WAL segments, in megabytes\n"));
 	printf(_("\nLess commonly used options:\n"));
+	printf(_("  -c, --set NAME=VALUE      override default setting for server parameter\n"));
 	printf(_("  -d, --debug               generate lots of debugging output\n"));
 	printf(_("      --discard-caches      set debug_discard_caches=1\n"));
 	printf(_("  -L DIRECTORY              where to find the input files\n"));
@@ -2759,6 +2975,7 @@ main(int argc, char *argv[])
 		{"nosync", no_argument, NULL, 'N'}, /* for backwards compatibility */
 		{"no-sync", no_argument, NULL, 'N'},
 		{"no-instructions", no_argument, NULL, 13},
+		{"set", required_argument, NULL, 'c'},
 		{"sync-only", no_argument, NULL, 'S'},
 		{"waldir", required_argument, NULL, 'X'},
 		{"wal-segsize", required_argument, NULL, 12},
@@ -2808,7 +3025,8 @@ main(int argc, char *argv[])
 
 	/* process command-line options */
 
-	while ((c = getopt_long(argc, argv, "A:dD:E:gkL:nNsST:U:WX:", long_options, &option_index)) != -1)
+	while ((c = getopt_long(argc, argv, "A:c:dD:E:gkL:nNsST:U:WX:",
+							long_options, &option_index)) != -1)
 	{
 		switch (c)
 		{
@@ -2831,6 +3049,24 @@ main(int argc, char *argv[])
 			case 11:
 				authmethodhost = pg_strdup(optarg);
 				break;
+			case 'c':
+				{
+					char	   *buf = pg_strdup(optarg);
+					char	   *equals = strchr(buf, '=');
+
+					if (!equals)
+					{
+						pg_log_error("-c %s requires a value", buf);
+						pg_log_error_hint("Try \"%s --help\" for more information.",
+										  progname);
+						exit(1);
+					}
+					*equals++ = '\0';	/* terminate variable name */
+					add_stringlist_item(&extra_guc_names, buf);
+					add_stringlist_item(&extra_guc_values, equals);
+					pfree(buf);
+				}
+				break;
 			case 'D':
 				pg_data = pg_strdup(optarg);
 				break;
diff --git a/src/bin/initdb/t/001_initdb.pl b/src/bin/initdb/t/001_initdb.pl
index 772769acab..1e23ac8d0c 100644
--- a/src/bin/initdb/t/001_initdb.pl
+++ b/src/bin/initdb/t/001_initdb.pl
@@ -48,7 +48,13 @@ mkdir $datadir;
 	local (%ENV) = %ENV;
 	delete $ENV{TZ};
 
-	command_ok([ 'initdb', '-N', '-T', 'german', '-X', $xlogdir, $datadir ],
+	# while we are here, also exercise -T and -c options
+	command_ok(
+		[
+			'initdb', '-N', '-T', 'german', '-c',
+			'default_text_search_config=german',
+			'-X', $xlogdir, $datadir
+		],
 		'successful creation');
 
 	# Permissions on PGDATA should be default
@@ -147,4 +153,7 @@ command_fails(
 	],
 	'fails for invalid option combination');
 
+command_fails([ 'initdb', '--no-sync', '--set', 'foo=bar', "$tempdir/dataX" ],
+	'fails for invalid --set option');
+
 done_testing();


^ permalink  raw  reply  [nested|flat] 8+ messages in thread

* Re: Set arbitrary GUC options during initdb
@ 2023-03-22 06:45  Peter Eisentraut <[email protected]>
  parent: Tom Lane <[email protected]>
  0 siblings, 0 replies; 8+ messages in thread

From: Peter Eisentraut @ 2023-03-22 06:45 UTC (permalink / raw)
  To: Tom Lane <[email protected]>; Robert Haas <[email protected]>; +Cc: David G. Johnston <[email protected]>; [email protected]

On 27.01.23 21:02, Tom Lane wrote:
> I wrote:
>>>> Anyway, it seems like I gotta work harder.  I'll produce a
>>>> new patch.
> 
> The string-hacking was fully as tedious as I expected.  However, the
> output looks pretty nice, and this does have the advantage that the
> pre-programmed substitutions become a lot more robust: they are no
> longer dependent on the initdb code exactly matching what is in
> postgresql.conf.sample.

This patch looks good to me.  It's a very nice simplification of the 
initdb.c code, even without the new feature.

I found that the addition of

#include <ctype.h>

didn't appear to be necessary.  Maybe it was required before 
guc_value_requires_quotes() was changed?

I would remove the

#if DEF_PGPORT != 5432

This was in the previous code too, but now if we remove it, then we 
don't have any more hardcoded 5432 left, which seems like a nice 
improvement in cleanliness.







^ permalink  raw  reply  [nested|flat] 8+ messages in thread

end of thread, other threads:[~2023-03-22 06:45 UTC | newest]

Thread overview: 8+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2020-10-19 11:24 [PATCH 2/2] Fast COPY FROM into the foreign or sharded table. Andrey V. Lepikhov <[email protected]>
2023-01-27 15:34 Re: Set arbitrary GUC options during initdb Tom Lane <[email protected]>
2023-01-27 15:41 ` Re: Set arbitrary GUC options during initdb Robert Haas <[email protected]>
2023-01-27 15:53   ` Re: Set arbitrary GUC options during initdb Tom Lane <[email protected]>
2023-01-27 16:01     ` Re: Set arbitrary GUC options during initdb David G. Johnston <[email protected]>
2023-01-27 16:35       ` Re: Set arbitrary GUC options during initdb Tom Lane <[email protected]>
2023-01-27 20:02         ` Re: Set arbitrary GUC options during initdb Tom Lane <[email protected]>
2023-03-22 06:45           ` Re: Set arbitrary GUC options during initdb Peter Eisentraut <[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