[PATCH v4 4/7] Row pattern recognition patch (executor).

public inbox for [email protected]  
help / color / mirror / Atom feed

[PATCH v4 4/7] Row pattern recognition patch (executor).
29+ messages / 7 participants
[nested] [flat]

* [PATCH v4 4/7] Row pattern recognition patch (executor).
@ 2023-08-09 07:56 Tatsuo Ishii <[email protected]>
  0 siblings, 0 replies; 29+ messages in thread

From: Tatsuo Ishii @ 2023-08-09 07:56 UTC (permalink / raw)

---
 src/backend/executor/nodeWindowAgg.c | 674 ++++++++++++++++++++++++++-
 src/backend/utils/adt/windowfuncs.c  |  38 +-
 src/include/catalog/pg_proc.dat      |   6 +
 src/include/nodes/execnodes.h        |  19 +
 src/include/windowapi.h              |   8 +
 5 files changed, 732 insertions(+), 13 deletions(-)

diff --git a/src/backend/executor/nodeWindowAgg.c b/src/backend/executor/nodeWindowAgg.c
index 310ac23e3a..5354e47045 100644
--- a/src/backend/executor/nodeWindowAgg.c
+++ b/src/backend/executor/nodeWindowAgg.c
@@ -36,6 +36,7 @@
 #include "access/htup_details.h"
 #include "catalog/objectaccess.h"
 #include "catalog/pg_aggregate.h"
+#include "catalog/pg_collation_d.h"
 #include "catalog/pg_proc.h"
 #include "executor/executor.h"
 #include "executor/nodeWindowAgg.h"
@@ -48,6 +49,7 @@
 #include "utils/acl.h"
 #include "utils/builtins.h"
 #include "utils/datum.h"
+#include "utils/fmgroids.h"
 #include "utils/expandeddatum.h"
 #include "utils/lsyscache.h"
 #include "utils/memutils.h"
@@ -182,8 +184,9 @@ static void begin_partition(WindowAggState *winstate);
 static void spool_tuples(WindowAggState *winstate, int64 pos);
 static void release_partition(WindowAggState *winstate);
 
-static int	row_is_in_frame(WindowAggState *winstate, int64 pos,
+static int  row_is_in_frame(WindowAggState *winstate, int64 pos,
 							TupleTableSlot *slot);
+
 static void update_frameheadpos(WindowAggState *winstate);
 static void update_frametailpos(WindowAggState *winstate);
 static void update_grouptailpos(WindowAggState *winstate);
@@ -195,9 +198,20 @@ static Datum GetAggInitVal(Datum textInitVal, Oid transtype);
 
 static bool are_peers(WindowAggState *winstate, TupleTableSlot *slot1,
 					  TupleTableSlot *slot2);
-static bool window_gettupleslot(WindowObject winobj, int64 pos,
-								TupleTableSlot *slot);
 
+static void attno_map(Node *node);
+static bool attno_map_walker(Node *node, void *context);
+static int row_is_in_reduced_frame(WindowObject winobj, int64 pos);
+
+static int64 evaluate_pattern(WindowObject winobj, int64 current_pos, 
+							  char *vname, StringInfo encoded_str, bool *result);
+
+static bool get_slots(WindowObject winobj, int64 current_pos);
+
+static int search_str_set(char *pattern, StringInfo *str_set, int set_size);
+static void search_str_set_recurse(char *pattern, StringInfo *str_set, int set_size, int set_index,
+								   char *encoded_str, int *resultlen);
+static char	pattern_initial(WindowAggState *winstate, char *vname);
 
 /*
  * initialize_windowaggregate
@@ -673,6 +687,9 @@ eval_windowaggregates(WindowAggState *winstate)
 	WindowObject agg_winobj;
 	TupleTableSlot *agg_row_slot;
 	TupleTableSlot *temp_slot;
+	bool		reduced_frame_set;
+	bool		check_reduced_frame;
+	int			num_rows_in_reduced_frame;
 
 	numaggs = winstate->numaggs;
 	if (numaggs == 0)
@@ -790,6 +807,7 @@ eval_windowaggregates(WindowAggState *winstate)
 			(winstate->frameOptions & FRAMEOPTION_EXCLUSION) ||
 			winstate->aggregatedupto <= winstate->frameheadpos)
 		{
+			elog(DEBUG1, "peraggstate->restart  is set");
 			peraggstate->restart = true;
 			numaggs_restart++;
 		}
@@ -861,8 +879,10 @@ eval_windowaggregates(WindowAggState *winstate)
 	 * If we created a mark pointer for aggregates, keep it pushed up to frame
 	 * head, so that tuplestore can discard unnecessary rows.
 	 */
+#ifdef NOT_USED
 	if (agg_winobj->markptr >= 0)
 		WinSetMarkPosition(agg_winobj, winstate->frameheadpos);
+#endif
 
 	/*
 	 * Now restart the aggregates that require it.
@@ -919,6 +939,10 @@ eval_windowaggregates(WindowAggState *winstate)
 		ExecClearTuple(agg_row_slot);
 	}
 
+	reduced_frame_set = false;
+	check_reduced_frame = false;
+	num_rows_in_reduced_frame = 0;
+
 	/*
 	 * Advance until we reach a row not in frame (or end of partition).
 	 *
@@ -930,12 +954,18 @@ eval_windowaggregates(WindowAggState *winstate)
 	{
 		int			ret;
 
+		elog(DEBUG1, "===== loop in frame starts: " INT64_FORMAT, winstate->aggregatedupto);
+
 		/* Fetch next row if we didn't already */
 		if (TupIsNull(agg_row_slot))
 		{
 			if (!window_gettupleslot(agg_winobj, winstate->aggregatedupto,
 									 agg_row_slot))
+			{
+				if (check_reduced_frame)
+					winstate->aggregatedupto--;
 				break;			/* must be end of partition */
+			}
 		}
 
 		/*
@@ -944,10 +974,47 @@ eval_windowaggregates(WindowAggState *winstate)
 		 */
 		ret = row_is_in_frame(winstate, winstate->aggregatedupto, agg_row_slot);
 		if (ret < 0)
+		{
+			if (winstate->patternVariableList != NIL && check_reduced_frame)
+				winstate->aggregatedupto--;
 			break;
+		}
 		if (ret == 0)
 			goto next_tuple;
 
+		if (winstate->patternVariableList != NIL)
+		{
+			if (!reduced_frame_set)
+			{
+				num_rows_in_reduced_frame = row_is_in_reduced_frame(winstate->agg_winobj, winstate->aggregatedupto);
+				reduced_frame_set = true;
+				elog(DEBUG1, "set num_rows_in_reduced_frame: %d pos: " INT64_FORMAT,
+					 num_rows_in_reduced_frame, winstate->aggregatedupto);
+
+				if (num_rows_in_reduced_frame <= 0)
+					break;
+
+				else if (num_rows_in_reduced_frame > 0)
+					check_reduced_frame = true;
+			}
+
+			if (check_reduced_frame)
+			{
+				elog(DEBUG1, "decrease num_rows_in_reduced_frame: %d pos: " INT64_FORMAT,
+					 num_rows_in_reduced_frame, winstate->aggregatedupto);
+				num_rows_in_reduced_frame--;
+				if (num_rows_in_reduced_frame < 0)
+				{
+					/*
+					 * No more rows remain in the reduced frame. Finish
+					 * accumulating row into the aggregates.
+					 */
+					winstate->aggregatedupto--;
+					break;
+				}
+			}
+		}
+
 		/* Set tuple context for evaluation of aggregate arguments */
 		winstate->tmpcontext->ecxt_outertuple = agg_row_slot;
 
@@ -976,6 +1043,8 @@ next_tuple:
 		ExecClearTuple(agg_row_slot);
 	}
 
+	elog(DEBUG1, "===== break loop in frame starts: " INT64_FORMAT, winstate->aggregatedupto);
+
 	/* The frame's end is not supposed to move backwards, ever */
 	Assert(aggregatedupto_nonrestarted <= winstate->aggregatedupto);
 
@@ -2053,6 +2122,8 @@ ExecWindowAgg(PlanState *pstate)
 
 	CHECK_FOR_INTERRUPTS();
 
+	elog(DEBUG1, "ExecWindowAgg called. pos: " INT64_FORMAT , winstate->currentpos);
+
 	if (winstate->status == WINDOWAGG_DONE)
 		return NULL;
 
@@ -2388,6 +2459,9 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
 	TupleDesc	scanDesc;
 	ListCell   *l;
 
+	TargetEntry	*te;
+	Expr		*expr;
+
 	/* check for unsupported flags */
 	Assert(!(eflags & (EXEC_FLAG_BACKWARD | EXEC_FLAG_MARK)));
 
@@ -2483,6 +2557,16 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
 	winstate->temp_slot_2 = ExecInitExtraTupleSlot(estate, scanDesc,
 												   &TTSOpsMinimalTuple);
 
+	winstate->prev_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+												 &TTSOpsMinimalTuple);
+
+	winstate->next_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+												 &TTSOpsMinimalTuple);
+
+	winstate->null_slot = ExecInitExtraTupleSlot(estate, scanDesc,
+												 &TTSOpsMinimalTuple);
+	winstate->null_slot = ExecStoreAllNullTuple(winstate->null_slot);
+
 	/*
 	 * create frame head and tail slots only if needed (must create slots in
 	 * exactly the same cases that update_frameheadpos and update_frametailpos
@@ -2667,6 +2751,39 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
 	winstate->inRangeAsc = node->inRangeAsc;
 	winstate->inRangeNullsFirst = node->inRangeNullsFirst;
 
+	/* Set up SKIP TO type */
+	winstate->rpSkipTo = node->rpSkipTo;
+	/* Set up row pattern recognition PATTERN clause */
+	winstate->patternVariableList = node->patternVariable;
+	winstate->patternRegexpList = node->patternRegexp;
+
+	/* Set up row pattern recognition DEFINE clause */
+	winstate->defineInitial = node->defineInitial;
+	winstate->defineVariableList = NIL;
+	winstate->defineClauseList = NIL;
+	if (node->defineClause != NIL)
+	{
+		/*
+		 * Tweak arg var of PREV/NEXT so that it refers to scan/inner slot.
+		 */
+		foreach(l, node->defineClause)
+		{
+			char		*name;
+			ExprState	*exps;
+
+			te = lfirst(l);
+			name = te->resname;
+			expr = te->expr;
+
+			elog(DEBUG1, "defineVariable name: %s", name);
+			winstate->defineVariableList = lappend(winstate->defineVariableList,
+												   makeString(pstrdup(name)));
+			attno_map((Node *)expr);
+			exps = ExecInitExpr(expr, (PlanState *) winstate);
+			winstate->defineClauseList = lappend(winstate->defineClauseList, exps);
+		}
+	}
+
 	winstate->all_first = true;
 	winstate->partition_spooled = false;
 	winstate->more_partitions = false;
@@ -2674,6 +2791,57 @@ ExecInitWindowAgg(WindowAgg *node, EState *estate, int eflags)
 	return winstate;
 }
 
+/*
+ * Rewrite varno of Var node that is the argument of PREV/NET so that it sees
+ * scan tuple (PREV) or inner tuple (NEXT).
+ */
+static void
+attno_map(Node *node)
+{
+	(void) expression_tree_walker(node, attno_map_walker, NULL);
+}
+
+static bool
+attno_map_walker(Node *node, void *context)
+{
+	FuncExpr	*func;
+	int			nargs;
+	Expr		*expr;
+	Var			*var;
+	
+	if (node == NULL)
+		return false;
+
+	if (IsA(node, FuncExpr))
+	{
+		func = (FuncExpr *)node;
+
+		if (func->funcid == F_PREV || func->funcid == F_NEXT)
+		{
+			/* sanity check */
+			nargs = list_length(func->args);
+			if (list_length(func->args) != 1)
+				elog(ERROR, "PREV/NEXT must have 1 argument but function %d has %d args", func->funcid, nargs);
+
+			expr = (Expr *) lfirst(list_head(func->args));
+			if (!IsA(expr, Var))
+				elog(ERROR, "PREV/NEXT's arg is not Var");	/* XXX: is it possible that arg type is Const? */
+			var = (Var *)expr;
+
+			if (func->funcid == F_PREV)
+				/*
+				 * Rewrite varno from OUTER_VAR to regular var no so that the
+				 * var references scan tuple.
+				 */
+				var->varno = var->varnosyn;
+			else
+				var->varno = INNER_VAR;
+			elog(DEBUG1, "PREV/NEXT's varno is rewritten to: %d", var->varno);
+		}
+	}
+	return expression_tree_walker(node, attno_map_walker, NULL);
+}
+
 /* -----------------
  * ExecEndWindowAgg
  * -----------------
@@ -2691,6 +2859,8 @@ ExecEndWindowAgg(WindowAggState *node)
 	ExecClearTuple(node->agg_row_slot);
 	ExecClearTuple(node->temp_slot_1);
 	ExecClearTuple(node->temp_slot_2);
+	ExecClearTuple(node->prev_slot);
+	ExecClearTuple(node->next_slot);
 	if (node->framehead_slot)
 		ExecClearTuple(node->framehead_slot);
 	if (node->frametail_slot)
@@ -2740,6 +2910,8 @@ ExecReScanWindowAgg(WindowAggState *node)
 	ExecClearTuple(node->agg_row_slot);
 	ExecClearTuple(node->temp_slot_1);
 	ExecClearTuple(node->temp_slot_2);
+	ExecClearTuple(node->prev_slot);
+	ExecClearTuple(node->next_slot);
 	if (node->framehead_slot)
 		ExecClearTuple(node->framehead_slot);
 	if (node->frametail_slot)
@@ -3080,7 +3252,7 @@ are_peers(WindowAggState *winstate, TupleTableSlot *slot1,
  *
  * Returns true if successful, false if no such row
  */
-static bool
+bool
 window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
 {
 	WindowAggState *winstate = winobj->winstate;
@@ -3100,7 +3272,7 @@ window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot)
 		return false;
 
 	if (pos < winobj->markpos)
-		elog(ERROR, "cannot fetch row before WindowObject's mark position");
+		elog(ERROR, "cannot fetch row: " INT64_FORMAT " before WindowObject's mark position: " INT64_FORMAT,  pos, winobj->markpos );
 
 	oldcontext = MemoryContextSwitchTo(winstate->ss.ps.ps_ExprContext->ecxt_per_query_memory);
 
@@ -3420,14 +3592,54 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
 	WindowAggState *winstate;
 	ExprContext *econtext;
 	TupleTableSlot *slot;
-	int64		abs_pos;
-	int64		mark_pos;
 
 	Assert(WindowObjectIsValid(winobj));
 	winstate = winobj->winstate;
 	econtext = winstate->ss.ps.ps_ExprContext;
 	slot = winstate->temp_slot_1;
 
+	if (WinGetSlotInFrame(winobj, slot,
+						  relpos, seektype, set_mark,
+						  isnull, isout) == 0)
+	{
+		econtext->ecxt_outertuple = slot;
+		return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
+							econtext, isnull);
+	}
+
+	if (isout)
+		*isout = true;
+	*isnull = true;
+	return (Datum) 0;
+}
+
+/*
+ * WinGetSlotInFrame
+ * slot: TupleTableSlot to store the result
+ * relpos: signed rowcount offset from the seek position
+ * seektype: WINDOW_SEEK_HEAD or WINDOW_SEEK_TAIL
+ * set_mark: If the row is found/in frame and set_mark is true, the mark is
+ *		moved to the row as a side-effect.
+ * isnull: output argument, receives isnull status of result
+ * isout: output argument, set to indicate whether target row position
+ *		is out of frame (can pass NULL if caller doesn't care about this)
+ *
+ * Returns 0 if we successfullt got the slot. false if out of frame.
+ * (also isout is set)
+ */
+int
+WinGetSlotInFrame(WindowObject winobj, TupleTableSlot *slot,
+					 int relpos, int seektype, bool set_mark,
+					 bool *isnull, bool *isout)
+{
+	WindowAggState *winstate;
+	int64		abs_pos;
+	int64		mark_pos;
+	int			num_reduced_frame;
+
+	Assert(WindowObjectIsValid(winobj));
+	winstate = winobj->winstate;
+
 	switch (seektype)
 	{
 		case WINDOW_SEEK_CURRENT:
@@ -3494,6 +3706,12 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
 						 winstate->frameOptions);
 					break;
 			}
+			num_reduced_frame = row_is_in_reduced_frame(winobj, winstate->frameheadpos);
+			if (num_reduced_frame < 0)
+				goto out_of_frame;
+			else if (num_reduced_frame > 0)
+				if (relpos >= num_reduced_frame)
+					goto out_of_frame;
 			break;
 		case WINDOW_SEEK_TAIL:
 			/* rejecting relpos > 0 is easy and simplifies code below */
@@ -3565,6 +3783,12 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
 					mark_pos = 0;	/* keep compiler quiet */
 					break;
 			}
+
+			num_reduced_frame = row_is_in_reduced_frame(winobj, winstate->frameheadpos + relpos);
+			if (num_reduced_frame < 0)
+				goto out_of_frame;
+			else if (num_reduced_frame > 0)
+				abs_pos = winstate->frameheadpos + relpos + num_reduced_frame - 1;
 			break;
 		default:
 			elog(ERROR, "unrecognized window seek type: %d", seektype);
@@ -3583,15 +3807,13 @@ WinGetFuncArgInFrame(WindowObject winobj, int argno,
 		*isout = false;
 	if (set_mark)
 		WinSetMarkPosition(winobj, mark_pos);
-	econtext->ecxt_outertuple = slot;
-	return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
-						econtext, isnull);
+	return 0;
 
 out_of_frame:
 	if (isout)
 		*isout = true;
 	*isnull = true;
-	return (Datum) 0;
+	return -1;
 }
 
 /*
@@ -3622,3 +3844,433 @@ WinGetFuncArgCurrent(WindowObject winobj, int argno, bool *isnull)
 	return ExecEvalExpr((ExprState *) list_nth(winobj->argstates, argno),
 						econtext, isnull);
 }
+
+WindowAggState *
+WinGetAggState(WindowObject winobj)
+{
+	return winobj->winstate;
+}
+
+/*
+ * row_is_in_reduced_frame
+ * Determine whether a row is in the current row's reduced window frame according
+ * to row pattern matching
+ *
+ * The row must has been already determined that it is in a full window frame
+ * and fetched it into slot.
+ *
+ * Returns:
+ * = 0, RPR is not defined.
+ * >0, if the row is the first in the reduced frame. Return the number of rows in the reduced frame.
+ * -1, if the row is unmatched row
+ * -2, if the row is in the reduced frame but needed to be skipped because of
+ * AFTER MATCH SKIP PAST LAST ROW
+ */
+static
+int row_is_in_reduced_frame(WindowObject winobj, int64 pos)
+{
+	WindowAggState *winstate = winobj->winstate;
+	ListCell	*lc1, *lc2;
+	bool		expression_result;
+	int			num_matched_rows;
+	int64		original_pos;
+	bool		anymatch;
+	StringInfo	encoded_str;
+	StringInfo	pattern_str = makeStringInfo();
+
+	/*
+	 * Array of pattern variables evaluted to true.
+	 * Each character corresponds to pattern variable.
+	 * Example:
+	 * str_set[0] = "AB";
+	 * str_set[1] = "AC";
+	 * In this case at row 0 A and B are true, and A and C are true in row 1.
+	 */
+	#define ENCODED_STR_ARRAY_ALLOC_SIZE	128
+	StringInfo	*str_set = NULL;
+	int		str_set_index;
+	int		str_set_size;
+
+	if (winstate->patternVariableList == NIL)
+	{
+		/*
+		 * RPR is not defined. Assume that we are always in the the reduced
+		 * window frame.
+		 */
+		return 0;
+	}
+
+	/* save original pos */
+	original_pos = pos;
+
+	/*
+	 * Check whether the row speicied by pos is in the reduced frame. The
+	 * second and subsequent rows need to be recognized as "unmatched" rows if
+	 * AFTER MATCH SKIP PAST LAST ROW is defined.
+	 */
+	if (winstate->rpSkipTo == ST_PAST_LAST_ROW &&
+		pos > winstate->headpos_in_reduced_frame &&
+		pos < (winstate->headpos_in_reduced_frame + winstate->num_rows_in_reduced_frame))
+		return -2;
+		
+	/*
+	 * Loop over until none of pattern matches or encounters end of frame.
+	 */
+	for (;;)
+	{
+		int64	result_pos = -1;
+
+		/*
+		 * Loop over each PATTERN variable.
+		 */
+		anymatch = false;
+		encoded_str = makeStringInfo();
+
+		forboth(lc1, winstate->patternVariableList, lc2, winstate->patternRegexpList)
+		{
+			char	*vname = strVal(lfirst(lc1));
+			char	*quantifier = strVal(lfirst(lc2));
+
+			elog(DEBUG1, "pos: " INT64_FORMAT " pattern vname: %s quantifier: %s", pos, vname, quantifier);
+
+			expression_result = false;
+
+			/* evaluate row pattern against current row */
+			result_pos = evaluate_pattern(winobj, pos, vname, encoded_str, &expression_result);
+			if (expression_result)
+			{
+				elog(DEBUG1, "expression result is true");
+				anymatch = true;
+			}
+
+			/*
+			 * If out of frame, we are done.
+			 */
+			 if (result_pos < 0)
+				 break;
+		}
+
+		if (!anymatch)
+		{
+			/* none of patterns matched. */
+			break;
+		}
+
+		/* build encoded string array */
+		if (str_set == NULL)
+		{
+			str_set_index = 0;
+			str_set_size = ENCODED_STR_ARRAY_ALLOC_SIZE * sizeof(StringInfo);
+			str_set = palloc(str_set_size);
+		}
+
+		str_set[str_set_index++] = encoded_str;
+
+		elog(DEBUG1, "pos: " INT64_FORMAT " str_set_index: %d encoded_str: %s", pos, str_set_index, encoded_str->data);
+
+		if (str_set_index >= str_set_size)
+		{
+			str_set_size *= 2;
+			str_set = repalloc(str_set, str_set_size);
+		}
+
+		/* move to next row */
+		pos++;
+
+		if (result_pos < 0)
+		{
+			/* out of frame */
+			break;
+		}
+	}
+
+	if (str_set == NULL)
+	{
+		/* no matches found in the first row */
+		return -1;
+	}
+
+	elog(DEBUG1, "pos: " INT64_FORMAT " encoded_str: %s", pos, encoded_str->data);
+
+	/* build regular expression */
+	pattern_str = makeStringInfo();
+	appendStringInfoChar(pattern_str, '^');
+	forboth (lc1, winstate->patternVariableList, lc2, winstate->patternRegexpList)
+	{
+		char	*vname = strVal(lfirst(lc1));
+		char	*quantifier = strVal(lfirst(lc2));
+		char	 initial;
+
+		initial = pattern_initial(winstate, vname);
+		Assert(initial != 0);
+		appendStringInfoChar(pattern_str, initial);
+		if (quantifier[0])
+			appendStringInfoChar(pattern_str, quantifier[0]);
+		elog(DEBUG1, "vname: %s initial: %c quantifier: %s", vname, initial, quantifier);
+	}
+
+	elog(DEBUG1, "pos: " INT64_FORMAT " pattern: %s", pos, pattern_str->data);
+
+	/* look for matching pattern variable sequence */
+	num_matched_rows = search_str_set(pattern_str->data, str_set, str_set_index);
+	if (num_matched_rows <= 0)
+		return -1;
+
+	/*
+	 * We are at the first row in the reduced frame.  Save the number of
+	 * matched rows as the number of rows in the reduced frame.
+	 */
+	winstate->headpos_in_reduced_frame = original_pos;
+	winstate->num_rows_in_reduced_frame = num_matched_rows;
+
+	return num_matched_rows;
+}
+
+/*
+ * search set of encode_str.
+ * set_size: size of set_str array.
+ */
+static
+int search_str_set(char *pattern, StringInfo *str_set, int set_size)
+{
+	char		*encoded_str = palloc0(set_size+1);
+	int			resultlen = 0;
+
+	search_str_set_recurse(pattern, str_set, set_size, 0, encoded_str, &resultlen);
+	elog(DEBUG1, "search_str_set returns %d", resultlen);
+	return resultlen;
+}
+
+static
+void search_str_set_recurse(char *pattern, StringInfo *str_set,
+							int set_size, int set_index, char *encoded_str, int *resultlen)
+{
+	char	*p;
+
+	if (set_index >= set_size)
+	{
+		Datum	d;
+		text	*res;
+		char	*substr;
+
+		/*
+		 * We first perform pattern matching using regexp_instr, then call
+		 * textregexsubstr to get matched substring to know how log the
+		 * matched string is. That is the number of rows in the reduced window
+		 * frame.  The reason why we can't call textregexsubstr is, it error
+		 * out if pattern is not match.
+		 */
+		if (DatumGetInt32(DirectFunctionCall2Coll(regexp_instr, DEFAULT_COLLATION_OID,
+												  PointerGetDatum(cstring_to_text(encoded_str)),
+												  PointerGetDatum(cstring_to_text(pattern)))) > 0)
+		{
+			d = DirectFunctionCall2Coll(textregexsubstr,
+										DEFAULT_COLLATION_OID,
+										PointerGetDatum(cstring_to_text(encoded_str)),
+										PointerGetDatum(cstring_to_text(pattern)));
+			if (d != 0)
+			{
+				int		len;
+
+				res = DatumGetTextPP(d);
+				substr = text_to_cstring(res);
+				len = strlen(substr);
+				if (len > *resultlen)
+					/* remember the longest match */
+					*resultlen = len;
+			}
+		}
+		return;
+	}
+
+	p = str_set[set_index]->data;
+	while (*p)
+	{
+		encoded_str[set_index] = *p;
+		p++;
+		search_str_set_recurse(pattern, str_set, set_size, set_index + 1, encoded_str, resultlen);
+	}
+}
+
+
+/*
+ * Evaluate expression associated with PATTERN variable vname.
+ * relpos is relative row position in a frame (starting from 0).
+ * "quantifier" is the quatifier part of the PATTERN regular expression.
+ * Currently only '+' is allowed.
+ * result is out paramater representing the expression evaluation result
+ * is true of false.
+ * Return values are:
+ * >=0: the last match absolute row position
+ * other wise out of frame.
+ */
+static
+int64 evaluate_pattern(WindowObject winobj, int64 current_pos, 
+						char *vname, StringInfo encoded_str, bool *result)
+{
+	WindowAggState	*winstate = winobj->winstate;
+	ExprContext		*econtext = winstate->ss.ps.ps_ExprContext;
+	ListCell		*lc1, *lc2, *lc3;
+	ExprState		*pat;
+	Datum			eval_result;
+	bool			out_of_frame = false;
+	bool			isnull;
+
+	forthree (lc1, winstate->defineVariableList, lc2, winstate->defineClauseList, lc3, winstate->defineInitial)
+	{
+		char	initial;
+		char	*name = strVal(lfirst(lc1));
+
+		elog(DEBUG1, "evaluate_pattern: define variable: %s, pattern variable: %s", name, vname);
+
+		if (strcmp(vname, name))
+			continue;
+
+		initial = *(strVal(lfirst(lc3)));
+
+		/* set expression to evaluate */
+		pat = lfirst(lc2);
+
+		/* get current, previous and next tuples */
+		if (!get_slots(winobj, current_pos))
+		{
+			out_of_frame = true;
+		}
+		else
+		{
+			/* evaluate the expression */
+			eval_result = ExecEvalExpr(pat, econtext, &isnull);
+			if (isnull)
+			{
+				/* expression is NULL */
+				elog(DEBUG1, "expression for %s is NULL at row: " INT64_FORMAT, vname, current_pos);
+				*result = false;
+			}
+			else
+			{
+				if (!DatumGetBool(eval_result))
+				{
+					/* expression is false */
+					elog(DEBUG1, "expression for %s is false at row: " INT64_FORMAT, vname, current_pos);
+					*result = false;
+				}
+				else
+				{
+					/* expression is true */
+					elog(DEBUG1, "expression for %s is true at row: " INT64_FORMAT, vname, current_pos);
+					appendStringInfoChar(encoded_str, initial);
+					*result = true;
+				}
+			}
+			break;
+		}
+
+		if (out_of_frame)
+		{
+			*result = false;
+			return -1;
+		}
+	}
+	return current_pos;
+}
+
+/*
+ * Get current, previous and next tuples.
+ * Returns false if current row is out of partition/full frame.
+ */
+static
+bool get_slots(WindowObject winobj, int64 current_pos)
+{
+	WindowAggState *winstate = winobj->winstate;
+	TupleTableSlot *slot;
+	int		ret;
+	ExprContext *econtext;
+
+	econtext = winstate->ss.ps.ps_ExprContext;
+
+	/* set up current row tuple slot */
+	slot = winstate->temp_slot_1;
+	if (!window_gettupleslot(winobj, current_pos, slot))
+	{
+		elog(DEBUG1, "current row is out of partition at:" INT64_FORMAT, current_pos);
+		return false;
+
+		ret = row_is_in_frame(winstate, current_pos, slot);
+		if (ret <= 0)
+		{
+			elog(DEBUG1, "current row is out of frame at: " INT64_FORMAT, current_pos);
+			return false;
+		}
+	}
+	econtext->ecxt_outertuple = slot;
+
+	/* for PREV */
+	if (current_pos > 0)
+	{
+		slot = winstate->prev_slot;
+		if (!window_gettupleslot(winobj, current_pos - 1, slot))
+		{
+			elog(DEBUG1, "previous row is out of partition at: " INT64_FORMAT, current_pos - 1);
+			econtext->ecxt_scantuple = winstate->null_slot;
+		}
+		else
+		{
+			ret = row_is_in_frame(winstate, current_pos - 1, slot);
+			if (ret <= 0)
+			{
+				elog(DEBUG1, "previous row is out of frame at: " INT64_FORMAT, current_pos - 1);
+				econtext->ecxt_scantuple = winstate->null_slot;
+			}
+			else
+			{
+				econtext->ecxt_scantuple = slot;
+			}
+		}
+	}
+	else
+		econtext->ecxt_scantuple = winstate->null_slot;
+
+	/* for NEXT */
+	slot = winstate->next_slot;
+	if (!window_gettupleslot(winobj, current_pos + 1, slot))
+	{
+		elog(DEBUG1, "next row is out of partiton at: " INT64_FORMAT, current_pos + 1);
+		econtext->ecxt_innertuple = winstate->null_slot;
+	}
+	else
+	{
+		ret = row_is_in_frame(winstate, current_pos + 1, slot);
+		if (ret <= 0)
+		{
+			elog(DEBUG1, "next row is out of frame at: " INT64_FORMAT, current_pos + 1);
+			econtext->ecxt_innertuple = winstate->null_slot;
+		}
+		else
+			econtext->ecxt_innertuple = slot;
+	}
+	return true;
+}
+
+/*
+ * Return pattern variable initial character
+ * matching with pattern variable name vname.
+ * If not found, return 0.
+ */
+static
+char	pattern_initial(WindowAggState *winstate, char *vname)
+{
+	char		initial;
+	char		*name;
+	ListCell	*lc1, *lc2;
+
+	forboth (lc1, winstate->defineVariableList, lc2, winstate->defineInitial)
+	{
+		name = strVal(lfirst(lc1));				/* DEFINE variable name */
+		initial = *(strVal(lfirst(lc2)));		/* DEFINE variable initial */
+
+
+		if (!strcmp(name, vname))
+				return initial;					/* found */
+	}
+	return 0;
+}
diff --git a/src/backend/utils/adt/windowfuncs.c b/src/backend/utils/adt/windowfuncs.c
index b87a624fb2..e4cab36ec9 100644
--- a/src/backend/utils/adt/windowfuncs.c
+++ b/src/backend/utils/adt/windowfuncs.c
@@ -13,6 +13,9 @@
  */
 #include "postgres.h"
 
+#include "catalog/pg_collation_d.h"
+#include "executor/executor.h"
+#include "nodes/execnodes.h"
 #include "nodes/supportnodes.h"
 #include "utils/builtins.h"
 #include "windowapi.h"
@@ -36,11 +39,19 @@ typedef struct
 	int64		remainder;		/* (total rows) % (bucket num) */
 } ntile_context;
 
+/*
+ * rpr process information.
+ * Used for AFTER MATCH SKIP PAST LAST ROW
+ */
+typedef struct SkipContext
+{
+	int64		pos;	/* last row absolute position */
+} SkipContext;
+
 static bool rank_up(WindowObject winobj);
 static Datum leadlag_common(FunctionCallInfo fcinfo,
 							bool forward, bool withoffset, bool withdefault);
 
-
 /*
  * utility routine for *_rank functions.
  */
@@ -673,7 +684,7 @@ window_last_value(PG_FUNCTION_ARGS)
 	bool		isnull;
 
 	result = WinGetFuncArgInFrame(winobj, 0,
-								  0, WINDOW_SEEK_TAIL, true,
+								  0, WINDOW_SEEK_TAIL, false,
 								  &isnull, NULL);
 	if (isnull)
 		PG_RETURN_NULL();
@@ -713,3 +724,26 @@ window_nth_value(PG_FUNCTION_ARGS)
 
 	PG_RETURN_DATUM(result);
 }
+
+/*
+ * prev
+ * Dummy function to invoke RPR's navigation operator "PREV".
+ * This is *not* a window function.
+ */
+Datum
+window_prev(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_DATUM(PG_GETARG_DATUM(0));
+}
+
+/*
+ * next
+ * Dummy function to invoke RPR's navigation operation "NEXT".
+ * This is *not* a window function.
+ */
+Datum
+window_next(PG_FUNCTION_ARGS)
+{
+	PG_RETURN_DATUM(PG_GETARG_DATUM(0));
+}
+
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 6996073989..fa100b2665 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10397,6 +10397,12 @@
 { oid => '3114', descr => 'fetch the Nth row value',
   proname => 'nth_value', prokind => 'w', prorettype => 'anyelement',
   proargtypes => 'anyelement int4', prosrc => 'window_nth_value' },
+{ oid => '6122', descr => 'previous value',
+  proname => 'prev', provolatile => 's', prorettype => 'anyelement',
+  proargtypes => 'anyelement', prosrc => 'window_prev' },
+{ oid => '6123', descr => 'next value',
+  proname => 'next', provolatile => 's', prorettype => 'anyelement',
+  proargtypes => 'anyelement', prosrc => 'window_next' },
 
 # functions for range types
 { oid => '3832', descr => 'I/O',
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index cb714f4a19..2bd6fcb5e1 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2519,6 +2519,15 @@ typedef struct WindowAggState
 	int64		groupheadpos;	/* current row's peer group head position */
 	int64		grouptailpos;	/* " " " " tail position (group end+1) */
 
+	/* these fields are used in Row pattern recognition: */
+	RPSkipTo	rpSkipTo;		/* Row Pattern Skip To type */	
+	List	   *patternVariableList;	/* list of row pattern variables names (list of String) */
+	List	   *patternRegexpList;	/* list of row pattern regular expressions ('+' or ''. list of String) */
+	List	   *defineVariableList;	/* list of row pattern definition variables (list of String) */
+	List	   *defineClauseList;	/* expression for row pattern definition
+									 * search conditions ExprState list */
+	List	   *defineInitial;		/* list of row pattern definition variable initials (list of String) */
+
 	MemoryContext partcontext;	/* context for partition-lifespan data */
 	MemoryContext aggcontext;	/* shared context for aggregate working data */
 	MemoryContext curaggcontext;	/* current aggregate's working data */
@@ -2555,6 +2564,16 @@ typedef struct WindowAggState
 	TupleTableSlot *agg_row_slot;
 	TupleTableSlot *temp_slot_1;
 	TupleTableSlot *temp_slot_2;
+
+	/* temporary slots for RPR */
+	TupleTableSlot *prev_slot;	/* PREV row navigation operator */
+	TupleTableSlot *next_slot;	/* NEXT row navigation operator */
+	TupleTableSlot *null_slot;	/* all NULL slot */
+
+	/* head of the reduced window frame */
+	int64		headpos_in_reduced_frame;
+	/* number of rows in the reduced window frame */
+	int64		num_rows_in_reduced_frame;
 } WindowAggState;
 
 /* ----------------
diff --git a/src/include/windowapi.h b/src/include/windowapi.h
index b8c2c565d1..1e292648e9 100644
--- a/src/include/windowapi.h
+++ b/src/include/windowapi.h
@@ -58,7 +58,15 @@ extern Datum WinGetFuncArgInFrame(WindowObject winobj, int argno,
 								  int relpos, int seektype, bool set_mark,
 								  bool *isnull, bool *isout);
 
+extern int WinGetSlotInFrame(WindowObject winobj, TupleTableSlot *slot,
+							 int relpos, int seektype, bool set_mark,
+							 bool *isnull, bool *isout);
+
 extern Datum WinGetFuncArgCurrent(WindowObject winobj, int argno,
 								  bool *isnull);
 
+extern WindowAggState *WinGetAggState(WindowObject winobj);
+
+extern bool window_gettupleslot(WindowObject winobj, int64 pos, TupleTableSlot *slot);
+
 #endif							/* WINDOWAPI_H */
-- 
2.25.1


----Next_Part(Wed_Aug__9_17_41_12_2023_134)--
Content-Type: Text/X-Patch; charset=us-ascii
Content-Transfer-Encoding: 7bit
Content-Disposition: inline;
 filename="v4-0005-Row-pattern-recognition-patch-docs.patch"



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

* Re: Flushing large data immediately in pqcomm
@ 2024-03-21 00:24 Melih Mutlu <[email protected]>
  2024-03-21 00:45 ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  0 siblings, 2 replies; 29+ messages in thread

From: Melih Mutlu @ 2024-03-21 00:24 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>

David Rowley <[email protected]>, 21 Mar 2024 Per, 00:54 tarihinde şunu
yazdı:

> On Fri, 15 Mar 2024 at 02:03, Jelte Fennema-Nio <[email protected]>
> wrote:
> >
> > On Thu, 14 Mar 2024 at 13:12, Robert Haas <[email protected]> wrote:
> > >
> > > On Thu, Mar 14, 2024 at 7:22 AM Melih Mutlu <[email protected]>
> wrote:
> > > > 1- Even though I expect both the patch and HEAD behave similarly in
> case of small data (case 1: 100 bytes), the patch runs slightly slower than
> HEAD.
> > >
> > > I wonder why this happens. It seems like maybe something that could be
> fixed.
> >
> > some wild guesses:
> > 1. maybe it's the extra call overhead of the new internal_flush
> > implementation. What happens if you make that an inline function?
> > 2. maybe swap these conditions around (the call seems heavier than a
> > simple comparison): !pq_is_send_pending() && len >= PqSendBufferSize
>
> I agree these are both worth trying.  For #2, I wonder if the
> pq_is_send_pending() call is even worth checking at all. It seems to
> me that the internal_flush_buffer() code will just do nothing if
> nothing is pending.  Also, isn't there almost always going to be
> something pending when the "len >= PqSendBufferSize" condition is met?
>  We've just added the msgtype and number of bytes to the buffer which
> is 5 bytes. If the previous message was also more than
> PqSendBufferSize, then the buffer is likely to have 5 bytes due to the
> previous flush, otherwise isn't it a 1 in 8192 chance that the buffer
> is empty?
>
> If that fails to resolve the regression, maybe it's worth memcpy()ing
> enough bytes out of the message to fill the buffer then flush it and
> check if we still have > PqSendBufferSize remaining and skip the
> memcpy() for the rest.  That way there are no small flushes of just 5
> bytes and only ever the possibility of reducing the flushes as no
> pattern should cause the number of flushes to increase.
>

In len > PqSendBufferSize cases, the buffer should be filled as much as
possible if we're sure that it will be flushed at some point. Otherwise we
might end up with small flushes. The cases where we're sure that the buffer
will be flushed is when the buffer is not empty. If it's empty, there is no
need to fill it unnecessarily as it might cause an additional flush. AFAIU
from what you said, we shouldn't be worried about such a case since it's
unlikely to have the buffer empty due to the first 5 bytes. I guess the
only case where the buffer can be empty is when the buffer has
PqSendBufferSize-5
bytes from previous messages and adding 5 bytes of the current message will
flush the buffer. I'm not sure if removing the check may cause any
regression in any case, but it's just there to be safe.

What if I do a simple comparison like PqSendStart == PqSendPointer instead
of calling pq_is_send_pending() as Heikki suggested, then this check should
not hurt that much. Right? Does that make sense?

-- 
Melih Mutlu
Microsoft


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-03-21 00:45 ` David Rowley <[email protected]>
  2024-03-21 09:44   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: David Rowley @ 2024-03-21 00:45 UTC (permalink / raw)
  To: Melih Mutlu <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>

On Thu, 21 Mar 2024 at 13:24, Melih Mutlu <[email protected]> wrote:
> What if I do a simple comparison like PqSendStart == PqSendPointer instead of calling pq_is_send_pending() as Heikki suggested, then this check should not hurt that much. Right? Does that make sense?

As I understand the code, there's no problem calling
internal_flush_buffer() when the buffer is empty and I suspect that if
we're sending a few buffers with "len > PqSendBufferSize" that it's
just so unlikely that the buffer is empty that we should just do the
function call and let internal_flush_buffer() handle doing nothing if
the buffer really is empty.  I think the chances of
internal_flush_buffer() having to do exactly nothing here is less than
1 in 8192, so I just don't think the check is worthwhile.  The reason
I don't think the odds are exactly 1 in 8192 is because if we're
sending a large number of bytes then it will be common that the buffer
will contain exactly 5 bytes due to the previous flush and command
prefix just having been added.

It's worth testing both, however. I might be wrong. Performance is
hard to predict. It would be good to see your test.sh script run with
and without the PqSendStart == PqSendPointer condition.

David

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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 00:45 ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-03-21 09:44   ` Jelte Fennema-Nio <[email protected]>
  2024-03-21 11:41     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-03-21 09:44 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Melih Mutlu <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>

On Thu, 21 Mar 2024 at 01:45, David Rowley <[email protected]> wrote:
> As I understand the code, there's no problem calling
> internal_flush_buffer() when the buffer is empty and I suspect that if
> we're sending a few buffers with "len > PqSendBufferSize" that it's
> just so unlikely that the buffer is empty that we should just do the
> function call and let internal_flush_buffer() handle doing nothing if
> the buffer really is empty.  I think the chances of
> internal_flush_buffer() having to do exactly nothing here is less than
> 1 in 8192, so I just don't think the check is worthwhile.

I think you're missing the exact case that we're trying to improve
here: Calls to internal_putbytes with a very large len, e.g. 1MB.
With the new code the buffer will be empty ~50% of the time (not less
than 1 in 8192) with such large buffers, because the flow that will
happen:

1. We check len > PqSendBufferSize. There are some bytes in the buffer
e.g. the 5 bytes of the msgtype. So we fill up the buffer, but have
many bytes left in len.
2. We loop again, because len is not 0.
3. We flush the buffer (at the top of the loop) because the buffer is full.
4. We check len > PqSendBufferSize. Now the buffer is empty, so we
call internal_flush_buffer directly

As you can see we check len > PqSendBufferSize twice (in step 1. and
step 4.), and 1 out of 2 times it returns 0

To be clear, the code is done this way so our behaviour would only
ever be better than the status-quo, and cause no regressions. For
instance, flushing the 5 byte header separately and then flushing the
full input buffer might result in more IP packets being sent in total
in some cases due to our TCP_NODELAY.

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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 00:45 ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-21 09:44   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-03-21 11:41     ` David Rowley <[email protected]>
  0 siblings, 0 replies; 29+ messages in thread

From: David Rowley @ 2024-03-21 11:41 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: Melih Mutlu <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>

On Thu, 21 Mar 2024 at 22:44, Jelte Fennema-Nio <[email protected]> wrote:
>
> On Thu, 21 Mar 2024 at 01:45, David Rowley <[email protected]> wrote:
> > As I understand the code, there's no problem calling
> > internal_flush_buffer() when the buffer is empty and I suspect that if
> > we're sending a few buffers with "len > PqSendBufferSize" that it's
> > just so unlikely that the buffer is empty that we should just do the
> > function call and let internal_flush_buffer() handle doing nothing if
> > the buffer really is empty.  I think the chances of
> > internal_flush_buffer() having to do exactly nothing here is less than
> > 1 in 8192, so I just don't think the check is worthwhile.
>
> I think you're missing the exact case that we're trying to improve
> here: Calls to internal_putbytes with a very large len, e.g. 1MB.
> With the new code the buffer will be empty ~50% of the time (not less
> than 1 in 8192) with such large buffers, because the flow that will
> happen:

It was the code I misread. I understand what the aim is. I failed to
notice the while loop in internal_putbytes().  So what I mentioned
about trying to fill the buffer before flushing already happens.  I
now agree that the PqSendStart == PqSendPointer test.  I'd say since
the reported regression was with 100 byte rows that testing "len >=
PqSendBufferSize" before PqSendStart == PqSendPointer makes sense.

David






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-03-21 09:58 ` Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-03-21 09:58 UTC (permalink / raw)
  To: Melih Mutlu <[email protected]>; +Cc: David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>

On Thu, 21 Mar 2024 at 01:24, Melih Mutlu <[email protected]> wrote:
> What if I do a simple comparison like PqSendStart == PqSendPointer instead of calling pq_is_send_pending()

Yeah, that sounds worth trying out. So the new suggestions to fix the
perf issues on small message sizes would be:

1. add "inline" to internal_flush function
2. replace pq_is_send_pending() with PqSendStart == PqSendPointer
3. (optional) swap the order of PqSendStart == PqSendPointer and len
>= PqSendBufferSize






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-03-21 23:45   ` Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: Melih Mutlu @ 2024-03-21 23:45 UTC (permalink / raw)
  To: PostgreSQL Hackers <[email protected]>; +Cc: David Rowley <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Jelte Fennema-Nio <[email protected]>

Hi,

PSA v3.

Jelte Fennema-Nio <[email protected]>, 21 Mar 2024 Per, 12:58 tarihinde
şunu yazdı:

> On Thu, 21 Mar 2024 at 01:24, Melih Mutlu <[email protected]> wrote:
> > What if I do a simple comparison like PqSendStart == PqSendPointer
> instead of calling pq_is_send_pending()
>
> Yeah, that sounds worth trying out. So the new suggestions to fix the
> perf issues on small message sizes would be:
>
> 1. add "inline" to internal_flush function
> 2. replace pq_is_send_pending() with PqSendStart == PqSendPointer
> 3. (optional) swap the order of PqSendStart == PqSendPointer and len
> >= PqSendBufferSize
>

I did all of the above changes and it seems like those resolved the
regression issue.
Since the previous results were with unix sockets, I share here the results
of v3 when using unix sockets for comparison.
Sharing only the case where all messages are 100 bytes, since this was when
the regression was most visible.

row size = 100 bytes, # of rows = 1000000
┌───────────┬────────────┬──────┬──────┬──────┬──────┬──────┐
│           │ 1400 bytes │ 2KB  │ 4KB  │ 8KB  │ 16KB │ 32KB │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ HEAD      │ 1106       │ 1006 │ 947  │ 920  │ 899  │ 888  │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ patch     │ 1094       │ 997  │ 943  │ 913  │ 894  │ 881  │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ no buffer │ 6389       │ 6195 │ 6214 │ 6271 │ 6325 │ 6211 │
└───────────┴────────────┴──────┴──────┴──────┴──────┴──────┘

David Rowley <[email protected]>, 21 Mar 2024 Per, 00:57 tarihinde şunu
yazdı:

> On Fri, 15 Mar 2024 at 01:46, Heikki Linnakangas <[email protected]> wrote:
> > - the "(int *) &len)" cast is not ok, and will break visibly on
> > big-endian systems where sizeof(int) != sizeof(size_t).
>
> I think fixing this requires adjusting the signature of
> internal_flush_buffer() to use size_t instead of int.   That also
> means that PqSendStart and PqSendPointer must also become size_t, or
> internal_flush() must add local size_t variables to pass to
> internal_flush_buffer and assign these back again to the global after
> the call.  Upgrading the globals might be the cleaner option.
>
> David


This is done too.

I actually tried to test it over a real network for a while. However, I
couldn't get reliable-enough numbers with both HEAD and the patch due to
network related issues.
I've decided to go with Jelte's suggestion [1]  which is decreasing MTU of
the loopback interface to 1500 and using localhost.

Here are the results:

1- row size = 100 bytes, # of rows = 1000000
┌───────────┬────────────┬───────┬───────┬───────┬───────┬───────┐
│           │ 1400 bytes │ 2KB   │  4KB  │  8KB  │  16KB │  32KB │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ HEAD      │ 1351       │ 1233  │ 1074  │  988  │  944  │  916  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ patch     │ 1369       │ 1232  │ 1073  │  981  │  928  │  907  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ no buffer │ 14949      │ 14533 │ 14791 │ 14864 │ 14612 │ 14751 │
└───────────┴────────────┴───────┴───────┴───────┴───────┴───────┘

2-  row size = half of the rows are 1KB and rest is 10KB , # of rows =
1000000
┌───────────┬────────────┬───────┬───────┬───────┬───────┬───────┐
│           │ 1400 bytes │ 2KB   │ 4KB   │ 8KB   │ 16KB  │ 32KB  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ HEAD      │ 37212      │ 31372 │ 25520 │ 21980 │ 20311 │ 18864 │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ patch     │ 23006      │ 23127 │ 23147 │ 22229 │ 20367 │ 19155 │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ no buffer │ 30725      │ 31090 │ 30917 │ 30796 │ 30984 │ 30813 │
└───────────┴────────────┴───────┴───────┴───────┴───────┴───────┘

3-  row size = half of the rows are 1KB and rest is 1MB , # of rows = 1000
┌───────────┬────────────┬──────┬──────┬──────┬──────┬──────┐
│           │ 1400 bytes │ 2KB  │ 4KB  │ 8KB  │ 16KB │ 32KB │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ HEAD      │ 4296       │ 3713 │ 3040 │ 2711 │ 2528 │ 2449 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ patch     │ 2401       │ 2411 │ 2404 │ 2374 │ 2395 │ 2408 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ no buffer │ 2399       │ 2403 │ 2408 │ 2389 │ 2402 │ 2403 │
└───────────┴────────────┴──────┴──────┴──────┴──────┴──────┘

4-  row size = all rows are 1MB , # of rows = 1000
┌───────────┬────────────┬──────┬──────┬──────┬──────┬──────┐
│           │ 1400 bytes │ 2KB  │ 4KB  │ 8KB  │ 16KB │ 32KB │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ HEAD      │ 8335       │ 7370 │ 6017 │ 5368 │ 5009 │ 4843 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ patch     │ 4711       │ 4722 │ 4708 │ 4693 │ 4724 │ 4717 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ no buffer │ 4704       │ 4712 │ 4746 │ 4728 │ 4709 │ 4730 │
└───────────┴────────────┴──────┴──────┴──────┴──────┴──────┘


[1]
https://www.postgresql.org/message-id/CAGECzQQMktuTj8ijJgBRXCwLEqfJyAFxg1h7rCTej-6%3DcR0r%3DQ%40mail...

Thanks,
-- 
Melih Mutlu
Microsoft


Attachments:

  [application/octet-stream] v3-0001-Flush-large-data-immediately-in-pqcomm.patch (4.2K, ../../CAGPVpCQ-265P-DY8hZADEKE5GO0-1NVB9kn7dH82BQgEUbdv1g@mail.gmail.com/3-v3-0001-Flush-large-data-immediately-in-pqcomm.patch)
  download | inline diff:
From ae73dd46b099cf7c21e4f1c725d12bfd4f3e45b1 Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 20 Nov 2023 11:20:52 +0300
Subject: [PATCH v3] Flush large data immediately in pqcomm

If the data is larger than send buffer size in pqcomm, we're sure that
the send buffer will be flushed at least once to fit the data depending
on how large the data is.

Instead of repeatedly calling memcpy and then flushing data larger than
the available space in the send buffer, this patch changes
internal_putbytes() logic to flush large data immediately if the buffer
is empty without unnecessarily copying it into the pqcomm send buffer.
---
 src/backend/libpq/pqcomm.c | 61 +++++++++++++++++++++++++++++---------
 1 file changed, 47 insertions(+), 14 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a..f663a0cfdd 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -120,8 +120,8 @@ static List *sock_paths = NIL;
 
 static char *PqSendBuffer;
 static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -145,6 +145,7 @@ static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
 static int	internal_putbytes(const char *s, size_t len);
 static int	internal_flush(void);
+static int	internal_flush_buffer(const char *s, size_t *start, size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1282,14 +1283,32 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			amount = PqSendBufferSize - PqSendPointer;
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1323,11 +1342,25 @@ socket_flush(void)
  */
 static int
 internal_flush(void)
+{
+	/* flush the pending output from send buffer. */
+	return internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer);
+}
+
+/* --------------------------------
+ *		internal_flush_buffer - flush the given buffer content
+ *
+ * Returns 0 if OK (meaning everything was sent, or operation would block
+ * and the socket is in non-blocking mode), or EOF if trouble.
+ * --------------------------------
+ */
+static int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	char	   *bufptr = (char*) s + *start;
+	char	   *bufend = (char*) s + *end;
 
 	while (bufptr < bufend)
 	{
@@ -1373,7 +1406,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1414,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-03-27 11:39     ` David Rowley <[email protected]>
  2024-03-27 15:54       ` Re: Flushing large data immediately in pqcomm Robert Haas <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  0 siblings, 2 replies; 29+ messages in thread

From: David Rowley @ 2024-03-27 11:39 UTC (permalink / raw)
  To: Melih Mutlu <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>; Robert Haas <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Jelte Fennema-Nio <[email protected]>

On Fri, 22 Mar 2024 at 12:46, Melih Mutlu <[email protected]> wrote:
> I did all of the above changes and it seems like those resolved the regression issue.

Thanks for adjusting the patch.   The numbers do look better, but on
looking at your test.sh script from [1], I see:

meson setup --buildtype debug -Dcassert=true
--prefix="$DESTDIR/usr/local/pgsql" $DESTDIR && \

can you confirm if the test was done in debug with casserts on?   If
so, it would be much better to have asserts off and have
-Dbuildtype=release.

I'm planning to run some benchmarks tomorrow.   My thoughts are that
the patch allows the memcpy() to be skipped without adding any
additional buffer flushes and demonstrates a good performance increase
in various scenarios from doing so.  I think that is a satisfactory
goal. If I don't see any issues from reviewing and benchmarking
tomorrow, I'd like to commit this.

Robert, I understand you'd like a bit more from this patch. I'm
wondering if you planning on blocking another committer from going
ahead with this? Or if you have a reason why the current state of the
patch is not a meaningful enough improvement that would justify
possibly not getting any improvements in this area for PG17?

David

[1] https://www.postgresql.org/message-id/CAGPVpCSX8bTF61ZL9jOgh1AaY3bgsWnQ6J7WmJK4TV0f2LPnJQ%40mail.gma...






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-03-27 15:54       ` Robert Haas <[email protected]>
  2024-03-28 19:47         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Robert Haas @ 2024-03-27 15:54 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Thomas Munro <[email protected]>; Jelte Fennema-Nio <[email protected]>

On Wed, Mar 27, 2024 at 7:39 AM David Rowley <[email protected]> wrote:
> Robert, I understand you'd like a bit more from this patch. I'm
> wondering if you planning on blocking another committer from going
> ahead with this? Or if you have a reason why the current state of the
> patch is not a meaningful enough improvement that would justify
> possibly not getting any improvements in this area for PG17?

So, I think that the first version of the patch, when it got a big
chunk of data, would just flush whatever was already in the buffer and
then send the rest without copying. The current version, as I
understand it, only does that if the buffer is empty; otherwise, it
copies data as much data as it can into the partially-filled buffer. I
think that change addresses most of my concern about the approach; the
old way could, I believe, lead to an increased total number of flushes
with the right usage pattern, but I don't believe that's possible with
the revised approach. I do kind of wonder whether there is some more
fine-tuning of the approach that would improve things further, but I
realize that we have very limited time to figure this out, and there's
no sense letting the perfect be the enemy of the good.

So in short... no, I don't have big concerns at this point. Melih's
latest benchmarks look fairly promising to me, too.

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

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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-27 15:54       ` Re: Flushing large data immediately in pqcomm Robert Haas <[email protected]>
@ 2024-03-28 19:47         ` Melih Mutlu <[email protected]>
  0 siblings, 0 replies; 29+ messages in thread

From: Melih Mutlu @ 2024-03-28 19:47 UTC (permalink / raw)
  To: Robert Haas <[email protected]>; +Cc: Andres Freund <[email protected]>; David Rowley <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Thomas Munro <[email protected]>

On Wed, Mar 27, 2024 at 18:54 Robert Haas <[email protected]> wrote:

> On Wed, Mar 27, 2024 at 7:39 AM David Rowley <[email protected]> wrote:
> > Robert, I understand you'd like a bit more from this patch. I'm
> > wondering if you planning on blocking another committer from going
> > ahead with this? Or if you have a reason why the current state of the
> > patch is not a meaningful enough improvement that would justify
> > possibly not getting any improvements in this area for PG17?
>
> So, I think that the first version of the patch, when it got a big
> chunk of data, would just flush whatever was already in the buffer and
> then send the rest without copying.


Correct.

The current version, as I
> understand it, only does that if the buffer is empty; otherwise, it
> copies data as much data as it can into the partially-filled buffer.


Yes, currently it should fill and flush the buffer first, if it’s not
already empty. Only then it sends the rest without copying.

Thanks,
Melih


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-03-28 19:44       ` Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Melih Mutlu @ 2024-03-28 19:44 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Wed, Mar 27, 2024 at 14:39 David Rowley <[email protected]> wrote:

> On Fri, 22 Mar 2024 at 12:46, Melih Mutlu <[email protected]> wrote:
> > I did all of the above changes and it seems like those resolved the
> regression issue.
>
> Thanks for adjusting the patch.   The numbers do look better, but on
> looking at your test.sh script from [1], I see:
>
> meson setup --buildtype debug -Dcassert=true
> --prefix="$DESTDIR/usr/local/pgsql" $DESTDIR && \
>
> can you confirm if the test was done in debug with casserts on?   If
> so, it would be much better to have asserts off and have
> -Dbuildtype=release.


Yes, previous numbers were with --buildtype debug -Dcassert=true. I can
share new numbers with release build and asserts off soon.

Thanks,
Melih


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-04-04 11:08         ` Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: Melih Mutlu @ 2024-04-04 11:08 UTC (permalink / raw)
  To: David Rowley <[email protected]>; PostgreSQL Hackers <[email protected]>; +Cc: Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Jelte Fennema-Nio <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Hi,

Melih Mutlu <[email protected]>, 28 Mar 2024 Per, 22:44 tarihinde şunu
yazdı:
>
> On Wed, Mar 27, 2024 at 14:39 David Rowley <[email protected]> wrote:
>>
>> On Fri, 22 Mar 2024 at 12:46, Melih Mutlu <[email protected]> wrote:
>> can you confirm if the test was done in debug with casserts on?   If
>> so, it would be much better to have asserts off and have
>> -Dbuildtype=release.
>
>
> Yes, previous numbers were with --buildtype debug -Dcassert=true. I can
share new numbers with release build and asserts off soon.

While testing the patch without --buildtype debug -Dcassert=true, I felt
like there was still a slight regression. I changed internal_flush() to an
inline function, results look better this way.


1- row size = 100 bytes, # of rows = 1000000
┌───────────┬────────────┬───────┬───────┬───────┬───────┬───────┐
│           │ 1400 bytes │ 2KB   │  4KB  │  8KB  │  16KB │  32KB │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ HEAD      │  861       │ 765   │ 612   │  521  │  477  │  480  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ patch     │  869       │ 766   │ 612   │  519  │  482  │  467  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ no buffer │ 13978      │ 13746 │ 13909 │ 13956 │ 13920 │ 13895 │
└───────────┴────────────┴───────┴───────┴───────┴───────┴───────┘

2-  row size = half of the rows are 1KB and rest is 10KB , # of rows =
1000000
┌───────────┬────────────┬───────┬───────┬───────┬───────┬───────┐
│           │ 1400 bytes │ 2KB   │ 4KB   │ 8KB   │ 16KB  │ 32KB  │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ HEAD      │ 30195      │ 26455 │ 17338 │ 14562 │ 12844 │ 11652 │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ patch     │ 14744      │ 15830 │ 15697 │ 14273 │ 12794 │ 11652 │
├───────────┼────────────┼───────┼───────┼───────┼───────┼───────┤
│ no buffer │ 24054      │ 23992 │ 24162 │ 23951 │ 23901 │ 23925 │
└───────────┴────────────┴───────┴───────┴───────┴───────┴───────┘

3-  row size = half of the rows are 1KB and rest is 1MB , # of rows = 1000
┌───────────┬────────────┬──────┬──────┬──────┬──────┬──────┐
│           │ 1400 bytes │ 2KB  │ 4KB  │ 8KB  │ 16KB │ 32KB │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ HEAD      │ 3546       │ 3029 │ 2373 │ 2032 │ 1873 │ 1806 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ patch     │ 1715       │ 1723 │ 1724 │ 1731 │ 1729 │ 1709 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ no buffer │ 1749       │ 1748 │ 1742 │ 1744 │ 1757 │ 1744 │
└───────────┴────────────┴──────┴──────┴──────┴──────┴──────┘

4-  row size = all rows are 1MB , # of rows = 1000
┌───────────┬────────────┬──────┬──────┬──────┬──────┬──────┐
│           │ 1400 bytes │ 2KB  │ 4KB  │ 8KB  │ 16KB │ 32KB │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ HEAD      │ 7089       │ 5987 │ 4697 │ 4048 │ 3737 │ 3523 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ patch     │ 3438       │ 3411 │ 3400 │ 3416 │ 3399 │ 3429 │
├───────────┼────────────┼──────┼──────┼──────┼──────┼──────┤
│ no buffer │ 3432       │ 3432 │ 3416 │ 3424 │ 3378 │ 3429 │
└───────────┴────────────┴──────┴──────┴──────┴──────┴──────┘

Thanks,
-- 
Melih Mutlu
Microsoft


Attachments:

  [application/octet-stream] v4-0001-Flush-large-data-immediately-in-pqcomm.patch (4.4K, ../../CAGPVpCRc1vE4reowMynubn09Y2KKuK3OdVrPySGUkgJwYmse7g@mail.gmail.com/3-v4-0001-Flush-large-data-immediately-in-pqcomm.patch)
  download | inline diff:
From ef94fceb7be1217f8f7c0d667a8b0945d5421535 Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 20 Nov 2023 11:20:52 +0300
Subject: [PATCH v4] Flush large data immediately in pqcomm

If the data is larger than send buffer size in pqcomm, we're sure that
the send buffer will be flushed at least once to fit the data depending
on how large the data is.

Instead of repeatedly calling memcpy and then flushing data larger than
the available space in the send buffer, this patch changes
internal_putbytes() logic to flush large data immediately if the buffer
is empty without unnecessarily copying it into the pqcomm send buffer.
---
 src/backend/libpq/pqcomm.c | 65 ++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a..1bc31f3cb4 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -120,8 +120,8 @@ static List *sock_paths = NIL;
 
 static char *PqSendBuffer;
 static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -144,7 +144,8 @@ static bool socket_is_send_pending(void);
 static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
 static int	internal_putbytes(const char *s, size_t len);
-static int	internal_flush(void);
+static inline int internal_flush(void);
+static int	internal_flush_buffer(const char *s, size_t *start, size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1282,14 +1283,32 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			amount = PqSendBufferSize - PqSendPointer;
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1321,13 +1340,27 @@ socket_flush(void)
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
  */
-static int
+static inline int
 internal_flush(void)
+{
+	/* flush the pending output from send buffer. */
+	return internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer);
+}
+
+/* --------------------------------
+ *		internal_flush_buffer - flush the given buffer content
+ *
+ * Returns 0 if OK (meaning everything was sent, or operation would block
+ * and the socket is in non-blocking mode), or EOF if trouble.
+ * --------------------------------
+ */
+static inline int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	char	   *bufptr = (char*) s + *start;
+	char	   *bufend = (char*) s + *end;
 
 	while (bufptr < bufend)
 	{
@@ -1373,7 +1406,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1414,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-04-04 13:34           ` Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-04 13:34 UTC (permalink / raw)
  To: Melih Mutlu <[email protected]>; +Cc: David Rowley <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Thu, 4 Apr 2024 at 13:08, Melih Mutlu <[email protected]> wrote:
> I changed internal_flush() to an inline function, results look better this way.

It seems you also change internal_flush_buffer to be inline (but only
in the actual function definition, not declaration at the top). I
don't think inlining internal_flush_buffer should be necessary to
avoid the perf regressions, i.e. internal_flush is adding extra
indirection compared to master and is only a single line, so that one
makes sense to inline.

Other than that the code looks good to me.

The new results look great.

One thing that is quite interesting about these results is that
increasing the buffer size results in even better performance (by
quite a bit). I don't think we can easily choose a perfect number, as
it seems to be a trade-off between memory usage and perf. But allowing
people to configure it through a GUC like in your second patchset
would be quite useful I think, especially because larger buffers could
be configured for connections that would benefit most for it (e.g.
replication connections or big COPYs).

I think your add-pq_send_buffer_size-GUC.patch is essentially what we
would need there but it would need some extra changes to actually be
merge-able:
1. needs docs
2. rename PQ_SEND_BUFFER_SIZE (at least make it not UPPER_CASE, but
maybe also remove the PQ_ prefix)
3. It's marked as PGC_USERSET, but there's no logic to grow/shrink it
after initial allocation






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-04 14:28             ` Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: Melih Mutlu @ 2024-04-04 14:28 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: David Rowley <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Jelte Fennema-Nio <[email protected]>, 4 Nis 2024 Per, 16:34 tarihinde
şunu yazdı:

> On Thu, 4 Apr 2024 at 13:08, Melih Mutlu <[email protected]> wrote:
> > I changed internal_flush() to an inline function, results look better
> this way.
>
> It seems you also change internal_flush_buffer to be inline (but only
> in the actual function definition, not declaration at the top). I
> don't think inlining internal_flush_buffer should be necessary to
> avoid the perf regressions, i.e. internal_flush is adding extra
> indirection compared to master and is only a single line, so that one
> makes sense to inline.
>

Right. It was a mistake, forgot to remove that. Fixed it in v5.



> Other than that the code looks good to me.
>
> The new results look great.
>
> One thing that is quite interesting about these results is that
> increasing the buffer size results in even better performance (by
> quite a bit). I don't think we can easily choose a perfect number, as
> it seems to be a trade-off between memory usage and perf. But allowing
> people to configure it through a GUC like in your second patchset
> would be quite useful I think, especially because larger buffers could
> be configured for connections that would benefit most for it (e.g.
> replication connections or big COPYs).
>
> I think your add-pq_send_buffer_size-GUC.patch is essentially what we
> would need there but it would need some extra changes to actually be
> merge-able:
> 1. needs docs
> 2. rename PQ_SEND_BUFFER_SIZE (at least make it not UPPER_CASE, but
> maybe also remove the PQ_ prefix)
> 3. It's marked as PGC_USERSET, but there's no logic to grow/shrink it
> after initial allocation
>

I agree that the GUC patch requires more work to be in good shape. I
created that for testing purposes. But if we decide to make the buffer size
customizable, then I'll start polishing up that patch and address your
suggestions.

One case that could benefit from increased COPY performance is table sync
of logical replication. It might make sense letting users to configure
buffer size to speed up table sync. I'm not sure what kind of problems this
GUC would bring though.

Thanks,
-- 
Melih Mutlu
Microsoft


Attachments:

  [application/octet-stream] v5-0001-Flush-large-data-immediately-in-pqcomm.patch (4.4K, ../../CAGPVpCQWUz=VGAjycSw_1aRLRXj5wp8hubZHq992KBxp25veSQ@mail.gmail.com/3-v5-0001-Flush-large-data-immediately-in-pqcomm.patch)
  download | inline diff:
From 1334c5f529d4d70d1eb99c190b605ddc8dd87e64 Mon Sep 17 00:00:00 2001
From: Melih Mutlu <[email protected]>
Date: Mon, 20 Nov 2023 11:20:52 +0300
Subject: [PATCH v5] Flush large data immediately in pqcomm

If the data is larger than send buffer size in pqcomm, we're sure that
the send buffer will be flushed at least once to fit the data depending
on how large the data is.

Instead of repeatedly calling memcpy and then flushing data larger than
the available space in the send buffer, this patch changes
internal_putbytes() logic to flush large data immediately if the buffer
is empty without unnecessarily copying it into the pqcomm send buffer.
---
 src/backend/libpq/pqcomm.c | 65 ++++++++++++++++++++++++++++----------
 1 file changed, 49 insertions(+), 16 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a..f18ddd5660 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -120,8 +120,8 @@ static List *sock_paths = NIL;
 
 static char *PqSendBuffer;
 static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -144,7 +144,8 @@ static bool socket_is_send_pending(void);
 static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
 static int	internal_putbytes(const char *s, size_t len);
-static int	internal_flush(void);
+static inline int internal_flush(void);
+static int	internal_flush_buffer(const char *s, size_t *start, size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1282,14 +1283,32 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			amount = PqSendBufferSize - PqSendPointer;
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1321,13 +1340,27 @@ socket_flush(void)
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
  */
-static int
+static inline int
 internal_flush(void)
+{
+	/* flush the pending output from send buffer. */
+	return internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer);
+}
+
+/* --------------------------------
+ *		internal_flush_buffer - flush the given buffer content
+ *
+ * Returns 0 if OK (meaning everything was sent, or operation would block
+ * and the socket is in non-blocking mode), or EOF if trouble.
+ * --------------------------------
+ */
+static int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	char	   *bufptr = (char*) s + *start;
+	char	   *bufend = (char*) s + *end;
 
 	while (bufptr < bufend)
 	{
@@ -1373,7 +1406,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1414,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
@ 2024-04-06 01:34               ` David Rowley <[email protected]>
  2024-04-06 10:16                 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 21:56                 ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  0 siblings, 3 replies; 29+ messages in thread

From: David Rowley @ 2024-04-06 01:34 UTC (permalink / raw)
  To: Melih Mutlu <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Fri, 5 Apr 2024 at 03:28, Melih Mutlu <[email protected]> wrote:
>
> Jelte Fennema-Nio <[email protected]>, 4 Nis 2024 Per, 16:34 tarihinde şunu yazdı:
>>
>> On Thu, 4 Apr 2024 at 13:08, Melih Mutlu <[email protected]> wrote:
>> > I changed internal_flush() to an inline function, results look better this way.
>>
>> It seems you also change internal_flush_buffer to be inline (but only
>> in the actual function definition, not declaration at the top). I
>> don't think inlining internal_flush_buffer should be necessary to
>> avoid the perf regressions, i.e. internal_flush is adding extra
>> indirection compared to master and is only a single line, so that one
>> makes sense to inline.
>
> Right. It was a mistake, forgot to remove that. Fixed it in v5.

I don't see any issues with v5, so based on the performance numbers
shown on this thread for the latest patch, it would make sense to push
it.  The problem is, I just can't recreate the performance numbers.

I've tried both on my AMD 3990x machine and an Apple M2 with a script
similar to the test.sh from above.  I mostly just stripped out the
buffer size stuff and adjusted the timing code to something that would
work with mac.

The script runs each copy 30 times and takes the average time,
reported here in seconds.

With AMD 3990x:

master
Run 100 100 5000000: 1.032264113 sec
Run 1024 10240 200000: 1.016229105 sec
Run 1024 1048576 2000: 1.242267116 sec
Run 1048576 1048576 1000: 1.245425089 sec

v5
Run 100 100 5000000: 1.068543053 sec
Run 1024 10240 200000: 1.026298571 sec
Run 1024 1048576 2000: 1.231169669 sec
Run 1048576 1048576 1000: 1.236355567 sec

With the M2 mini:

master
Run 100 100 5000000: 1.167851249 sec
Run 1024 10240 200000: 1.962466987 sec
Run 1024 1048576 2000: 2.052836275 sec
Run 1048576 1048576 1000: 2.057908066 sec

v5
Run 100 100 5000000: 1.149636571 sec
Run 1024 10240 200000: 2.158487741 sec
Run 1024 1048576 2000: 2.046627068 sec
Run 1048576 1048576 1000: 2.039329068 sec

From looking at the perf reports, the top function is:

  57.62%  postgres   [.] CopyAttributeOutText

I messed around with trying to speed up the string escaping in that
function with the attached hacky patch and got the following on the
AMD 3990x machine:

CopyAttributeOutText_speedup.patch.txt
Run 100 100 5000000: 0.821673910
Run 1024 10240 200000: 0.546632147
Run 1024 1048576 2000: 0.848492694
Run 1048576 1048576 1000: 0.840870293

I don't think we could actually do this unless we modified the output
function API to have it somehow output the number of bytes. The patch
may look beyond the NUL byte with pg_lfind8, which I don't think is
safe.

Does anyone else want to try the attached script on the v5 patch to
see if their numbers are better?

David

#!/bin/bash

dbname=postgres
port=5432

test_cases=(
"100 100 5000000"		# only 100 bytes	
"1024 10240 200000"    # 1Kb and 10Kb 
"1024 1048576 2000"		# 1Kb and 1Mb 
"1048576 1048576 1000"	# all 1Mb
)

insert_rows(){
	psql -d $dbname -p $port -c "
	DO \$\$
	DECLARE
	    counter INT;
	BEGIN
	    FOR counter IN 1..$3 LOOP
	        IF counter % 2 = 1 THEN
	            INSERT INTO test_table VALUES (repeat('a', $1)::text);
	        ELSE
	            INSERT INTO test_table VALUES (repeat('b', $2)::text);
	        END IF;
	    END LOOP;
	END \$\$;
	" > /dev/null
}


psql -d $dbname -p $port -c "CREATE EXTENSION IF NOT EXISTS pg_prewarm;" > /dev/null

for case in "${test_cases[@]}" 
do
	psql -d $dbname -p $port -c "DROP TABLE IF EXISTS test_table;" > /dev/null
	psql -d $dbname -p $port -c "CREATE TABLE test_table(data text not null);" > /dev/null
	insert_rows $case

	psql -d $dbname -p $port -c "select pg_prewarm('test_table');" > /dev/null

	echo -n "Run $case: "

	elapsed_time=0
	for a in {1..30}
	do
		start_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		psql -d $dbname -p $port -c "COPY test_table TO STDOUT;" > /dev/null
		end_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		elapsed_time=$(perl -e "printf('%.9f', ($end_time - $start_time) + $elapsed_time)")
	done
	
	avg_elapsed_time_in_ms=$(perl -e "printf('%.9f', ($elapsed_time / 30))")
	echo $avg_elapsed_time_in_ms
done


diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index ae8b2e36d7..fcb200503f 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -29,6 +29,7 @@
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "pgstat.h"
+#include "port/pg_lfind.h"
 #include "storage/fd.h"
 #include "tcop/tcopprot.h"
 #include "utils/lsyscache.h"
@@ -1068,9 +1069,18 @@ CopyAttributeOutText(CopyToState cstate, const char *string)
 	else
 	{
 		start = ptr;
-		while ((c = *ptr) != '\0')
+
+		while (true)
 		{
-			if ((unsigned char) c < (unsigned char) 0x20)
+			while (!pg_lfind8('\\', (uint8 *) ptr, sizeof(Vector8)) &&
+				   !pg_lfind8(delimc, (uint8 *) ptr, sizeof(Vector8)) &&
+				   !pg_lfind8_le(31, (uint8 *) ptr, sizeof(Vector8)))
+				ptr += sizeof(Vector8);
+
+			if ((c = *ptr) == '\0')
+				break;
+
+			if ((unsigned char) c <= (unsigned char) 31)
 			{
 				/*
 				 * \r and \n must be escaped, the others are traditional. We


Attachments:

  [text/plain] test1.sh.txt (1.4K, ../../CAApHDvomWMAHVEUJL0bXE4Og9qwAeqdehP9gcWS6Px0yc6aH+Q@mail.gmail.com/2-test1.sh.txt)
  download | inline:
#!/bin/bash

dbname=postgres
port=5432

test_cases=(
"100 100 5000000"		# only 100 bytes	
"1024 10240 200000"    # 1Kb and 10Kb 
"1024 1048576 2000"		# 1Kb and 1Mb 
"1048576 1048576 1000"	# all 1Mb
)

insert_rows(){
	psql -d $dbname -p $port -c "
	DO \$\$
	DECLARE
	    counter INT;
	BEGIN
	    FOR counter IN 1..$3 LOOP
	        IF counter % 2 = 1 THEN
	            INSERT INTO test_table VALUES (repeat('a', $1)::text);
	        ELSE
	            INSERT INTO test_table VALUES (repeat('b', $2)::text);
	        END IF;
	    END LOOP;
	END \$\$;
	" > /dev/null
}


psql -d $dbname -p $port -c "CREATE EXTENSION IF NOT EXISTS pg_prewarm;" > /dev/null

for case in "${test_cases[@]}" 
do
	psql -d $dbname -p $port -c "DROP TABLE IF EXISTS test_table;" > /dev/null
	psql -d $dbname -p $port -c "CREATE TABLE test_table(data text not null);" > /dev/null
	insert_rows $case

	psql -d $dbname -p $port -c "select pg_prewarm('test_table');" > /dev/null

	echo -n "Run $case: "

	elapsed_time=0
	for a in {1..30}
	do
		start_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		psql -d $dbname -p $port -c "COPY test_table TO STDOUT;" > /dev/null
		end_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		elapsed_time=$(perl -e "printf('%.9f', ($end_time - $start_time) + $elapsed_time)")
	done
	
	avg_elapsed_time_in_ms=$(perl -e "printf('%.9f', ($elapsed_time / 30))")
	echo $avg_elapsed_time_in_ms
done


  [text/plain] CopyAttributeOutText_speedup.patch.txt (983B, ../../CAApHDvomWMAHVEUJL0bXE4Og9qwAeqdehP9gcWS6Px0yc6aH+Q@mail.gmail.com/3-CopyAttributeOutText_speedup.patch.txt)
  download | inline diff:
diff --git a/src/backend/commands/copyto.c b/src/backend/commands/copyto.c
index ae8b2e36d7..fcb200503f 100644
--- a/src/backend/commands/copyto.c
+++ b/src/backend/commands/copyto.c
@@ -29,6 +29,7 @@
 #include "mb/pg_wchar.h"
 #include "miscadmin.h"
 #include "pgstat.h"
+#include "port/pg_lfind.h"
 #include "storage/fd.h"
 #include "tcop/tcopprot.h"
 #include "utils/lsyscache.h"
@@ -1068,9 +1069,18 @@ CopyAttributeOutText(CopyToState cstate, const char *string)
 	else
 	{
 		start = ptr;
-		while ((c = *ptr) != '\0')
+
+		while (true)
 		{
-			if ((unsigned char) c < (unsigned char) 0x20)
+			while (!pg_lfind8('\\', (uint8 *) ptr, sizeof(Vector8)) &&
+				   !pg_lfind8(delimc, (uint8 *) ptr, sizeof(Vector8)) &&
+				   !pg_lfind8_le(31, (uint8 *) ptr, sizeof(Vector8)))
+				ptr += sizeof(Vector8);
+
+			if ((c = *ptr) == '\0')
+				break;
+
+			if ((unsigned char) c <= (unsigned char) 31)
 			{
 				/*
 				 * \r and \n must be escaped, the others are traditional. We


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-04-06 10:16                 ` Jelte Fennema-Nio <[email protected]>
  2024-04-06 12:51                   ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2 siblings, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-06 10:16 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Sat, 6 Apr 2024 at 03:34, David Rowley <[email protected]> wrote:
> Does anyone else want to try the attached script on the v5 patch to
> see if their numbers are better?

On my machine (i9-10900X, in Ubuntu 22.04 on WSL on Windows) v5
consistently beats master by ~0.25 seconds:

master:
Run 100 100 5000000: 1.948975205
Run 1024 10240 200000: 3.039986587
Run 1024 1048576 2000: 2.444176276
Run 1048576 1048576 1000: 2.475328596

v5:
Run 100 100 5000000: 1.997170909
Run 1024 10240 200000: 3.057802598
Run 1024 1048576 2000: 2.199449857
Run 1048576 1048576 1000: 2.210328762

The first two runs are pretty much equal, and I ran your script a few
more times and this seems like just random variance (sometimes v5 wins
those, sometimes master does always quite close to each other). But
the last two runs v5 consistently wins.

Weird that on your machines you don't see a difference. Are you sure
you didn't make a silly mistake, like not restarting postgres or
something?






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 10:16                 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-06 12:51                   ` David Rowley <[email protected]>
  0 siblings, 0 replies; 29+ messages in thread

From: David Rowley @ 2024-04-06 12:51 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Sat, 6 Apr 2024 at 23:17, Jelte Fennema-Nio <[email protected]> wrote:
> Weird that on your machines you don't see a difference. Are you sure
> you didn't make a silly mistake, like not restarting postgres or
> something?

I'm sure. I spent quite a long time between the AMD and an Apple m2 trying.

I did see the same regression as you on the smaller numbers.  I
experimented with the attached which macro'ifies internal_flush() and
pg_noinlines internal_flush_buffer.

Can you try that to see if it gets rid of the regression on the first two tests?

David

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a..824b2f11a3 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -120,8 +120,8 @@ static List *sock_paths = NIL;
 
 static char *PqSendBuffer;
 static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -133,6 +133,7 @@ static int	PqRecvLength;		/* End of data available in PqRecvBuffer */
 static bool PqCommBusy;			/* busy sending data to the client */
 static bool PqCommReadingMsg;	/* in the middle of reading a message */
 
+#define internal_flush()	internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer)
 
 /* Internal functions */
 static void socket_comm_reset(void);
@@ -144,7 +145,8 @@ static bool socket_is_send_pending(void);
 static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
 static int	internal_putbytes(const char *s, size_t len);
-static int	internal_flush(void);
+static pg_noinline int internal_flush_buffer(const char *s, size_t *start,
+											 size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1282,14 +1284,32 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			amount = PqSendBufferSize - PqSendPointer;
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1315,19 +1335,19 @@ socket_flush(void)
 }
 
 /* --------------------------------
- *		internal_flush - flush pending output
+ *		internal_flush_buffer - flush the given buffer content
  *
  * Returns 0 if OK (meaning everything was sent, or operation would block
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
  */
-static int
-internal_flush(void)
+static pg_noinline int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	char	   *bufptr = (char*) s + *start;
+	char	   *bufend = (char*) s + *end;
 
 	while (bufptr < bufend)
 	{
@@ -1373,7 +1393,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1401,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 


Attachments:

  [text/plain] flush_macro_noinline.patch (3.8K, ../../CAApHDvrg7MbzcgVY4LF3aRUYR57ox3K5VycE7NytbgMT=n37zw@mail.gmail.com/2-flush_macro_noinline.patch)
  download | inline diff:
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a..824b2f11a3 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -120,8 +120,8 @@ static List *sock_paths = NIL;
 
 static char *PqSendBuffer;
 static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -133,6 +133,7 @@ static int	PqRecvLength;		/* End of data available in PqRecvBuffer */
 static bool PqCommBusy;			/* busy sending data to the client */
 static bool PqCommReadingMsg;	/* in the middle of reading a message */
 
+#define internal_flush()	internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer)
 
 /* Internal functions */
 static void socket_comm_reset(void);
@@ -144,7 +145,8 @@ static bool socket_is_send_pending(void);
 static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
 static int	internal_putbytes(const char *s, size_t len);
-static int	internal_flush(void);
+static pg_noinline int internal_flush_buffer(const char *s, size_t *start,
+											 size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1282,14 +1284,32 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			amount = PqSendBufferSize - PqSendPointer;
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1315,19 +1335,19 @@ socket_flush(void)
 }
 
 /* --------------------------------
- *		internal_flush - flush pending output
+ *		internal_flush_buffer - flush the given buffer content
  *
  * Returns 0 if OK (meaning everything was sent, or operation would block
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
  */
-static int
-internal_flush(void)
+static pg_noinline int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	char	   *bufptr = (char*) s + *start;
+	char	   *bufend = (char*) s + *end;
 
 	while (bufptr < bufend)
 	{
@@ -1373,7 +1393,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1401,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-04-06 20:21                 ` Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 09:33                   ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2 siblings, 2 replies; 29+ messages in thread

From: Andres Freund @ 2024-04-06 20:21 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Melih Mutlu <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Hi,

On 2024-04-06 14:34:17 +1300, David Rowley wrote:
> I don't see any issues with v5, so based on the performance numbers
> shown on this thread for the latest patch, it would make sense to push
> it.  The problem is, I just can't recreate the performance numbers.
>
> I've tried both on my AMD 3990x machine and an Apple M2 with a script
> similar to the test.sh from above.  I mostly just stripped out the
> buffer size stuff and adjusted the timing code to something that would
> work with mac.

I think there are a few issues with the test script leading to not seeing a
gain:

1) I think using the textual protocol, with the text datatype, will make it
   harder to spot differences. That's a lot of overhead.

2) Afaict the test is connecting over the unix socket, I think we expect
   bigger wins for tcp

3) Particularly the larger string is bottlenecked due to pglz compression in
   toast.


Where I had noticed the overhead of the current approach badly, was streaming
out basebackups. Which is all binary, of course.


I added WITH BINARY, SET STORAGE EXTERNAL and tested both unix socket and
localhost. I also reduced row counts and iteration counts, because I am
impatient, and I don't think it matters much here. Attached the modified
version.


On a dual xeon Gold 5215, turbo boost disabled, server pinned to one core,
script pinned to another:


unix:

master:
Run 100 100 1000000: 0.058482377
Run 1024 10240 100000: 0.120909810
Run 1024 1048576 2000: 0.153027916
Run 1048576 1048576 1000: 0.154953512

v5:
Run 100 100 1000000: 0.058760126
Run 1024 10240 100000: 0.118831396
Run 1024 1048576 2000: 0.124282503
Run 1048576 1048576 1000: 0.123894962


localhost:

master:
Run 100 100 1000000: 0.067088000
Run 1024 10240 100000: 0.170894273
Run 1024 1048576 2000: 0.230346632
Run 1048576 1048576 1000: 0.230336078

v5:
Run 100 100 1000000: 0.067144036
Run 1024 10240 100000: 0.167950948
Run 1024 1048576 2000: 0.135167027
Run 1048576 1048576 1000: 0.135347867


The perf difference for 1MB via TCP is really impressive.

The small regression for small results is still kinda visible, I haven't yet
tested the patch downthread.

Greetings,

Andres Freund

#!/bin/bash

set -e

dbname=postgres
port=5440
host=/tmp
host=localhost

test_cases=(
"100 100 1000000"		# only 100 bytes
"1024 10240 100000"    # 1Kb and 10Kb
"1024 1048576 2000"		# 1Kb and 1Mb
"1048576 1048576 1000"	# all 1Mb
)

insert_rows(){
	psql -d $dbname -p $port -h $host  -c "
	DO \$\$
	DECLARE
	    counter INT;
	BEGIN
	    FOR counter IN 1..$3 LOOP
	        IF counter % 2 = 1 THEN
	            INSERT INTO test_table VALUES (repeat('a', $1)::text);
	        ELSE
	            INSERT INTO test_table VALUES (repeat('b', $2)::text);
	        END IF;
	    END LOOP;
	END \$\$;
	" > /dev/null
}


psql -d $dbname -p $port -c "CREATE EXTENSION IF NOT EXISTS pg_prewarm;" > /dev/null

for case in "${test_cases[@]}"
do
	psql -d $dbname -p $port -h $host -c "DROP TABLE IF EXISTS test_table;" > /dev/null
	psql -d $dbname -p $port -h $host  -c "CREATE UNLOGGED TABLE test_table(data text not null);" > /dev/null
	psql -d $dbname -p $port -h $host  -c "ALTER TABLE test_table ALTER data SET STORAGE EXTERNAL;" > /dev/null

	insert_rows $case

	psql -d $dbname -p $port -h $host  -c "select pg_prewarm('test_table');" > /dev/null

	echo -n "Run $case: "

	elapsed_time=0
	for a in {1..5}
	do
		start_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		psql -d $dbname -p $port -h $host -c "COPY test_table TO STDOUT WITH BINARY;" > /dev/null
		end_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		elapsed_time=$(perl -e "printf('%.9f', ($end_time - $start_time) + $elapsed_time)")
	done

	avg_elapsed_time_in_ms=$(perl -e "printf('%.9f', ($elapsed_time / 30))")
	echo $avg_elapsed_time_in_ms
done


Attachments:

  [text/plain] test1a.sh.txt (1.6K, ../../[email protected]/2-test1a.sh.txt)
  download | inline:
#!/bin/bash

set -e

dbname=postgres
port=5440
host=/tmp
host=localhost

test_cases=(
"100 100 1000000"		# only 100 bytes
"1024 10240 100000"    # 1Kb and 10Kb
"1024 1048576 2000"		# 1Kb and 1Mb
"1048576 1048576 1000"	# all 1Mb
)

insert_rows(){
	psql -d $dbname -p $port -h $host  -c "
	DO \$\$
	DECLARE
	    counter INT;
	BEGIN
	    FOR counter IN 1..$3 LOOP
	        IF counter % 2 = 1 THEN
	            INSERT INTO test_table VALUES (repeat('a', $1)::text);
	        ELSE
	            INSERT INTO test_table VALUES (repeat('b', $2)::text);
	        END IF;
	    END LOOP;
	END \$\$;
	" > /dev/null
}


psql -d $dbname -p $port -c "CREATE EXTENSION IF NOT EXISTS pg_prewarm;" > /dev/null

for case in "${test_cases[@]}"
do
	psql -d $dbname -p $port -h $host -c "DROP TABLE IF EXISTS test_table;" > /dev/null
	psql -d $dbname -p $port -h $host  -c "CREATE UNLOGGED TABLE test_table(data text not null);" > /dev/null
	psql -d $dbname -p $port -h $host  -c "ALTER TABLE test_table ALTER data SET STORAGE EXTERNAL;" > /dev/null

	insert_rows $case

	psql -d $dbname -p $port -h $host  -c "select pg_prewarm('test_table');" > /dev/null

	echo -n "Run $case: "

	elapsed_time=0
	for a in {1..5}
	do
		start_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		psql -d $dbname -p $port -h $host -c "COPY test_table TO STDOUT WITH BINARY;" > /dev/null
		end_time=$(perl -MTime::HiRes=time -e 'printf "%.9f\n", time')
		elapsed_time=$(perl -e "printf('%.9f', ($end_time - $start_time) + $elapsed_time)")
	done

	avg_elapsed_time_in_ms=$(perl -e "printf('%.9f', ($elapsed_time / 30))")
	echo $avg_elapsed_time_in_ms
done

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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
@ 2024-04-06 22:45                   ` Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-06 22:45 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: David Rowley <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

On Sat, 6 Apr 2024 at 22:21, Andres Freund <[email protected]> wrote:
> The small regression for small results is still kinda visible, I haven't yet
> tested the patch downthread.

Thanks a lot for the faster test script, I'm also impatient. I still
saw the small regression with David his patch. Here's a v6 where I
think it is now gone. I added inline to internal_put_bytes too. I
think that helped especially because for two calls to
internal_put_bytes len is a constant (1 and 4) that is smaller than
PqSendBufferSize. So for those calls the compiler can now statically
eliminate the new codepath because "len >= PqSendBufferSize" is known
to be false at compile time.

Also I incorporated all of Ranier his comments.


Attachments:

  [application/octet-stream] v6-0001-Faster-internal_putbytes.patch (5.0K, ../../CAGECzQRNBaGu8TrUk+oAnTSd4r1-Osb7a8R6w8FJDzBhKTpeyA@mail.gmail.com/2-v6-0001-Faster-internal_putbytes.patch)
  download | inline diff:
From a25bd7013ba491ebc894f5c5dfba3a751ab501c0 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Sat, 6 Apr 2024 19:19:28 +0200
Subject: [PATCH v6] Faster internal_putbytes

---
 src/backend/libpq/pqcomm.c | 69 ++++++++++++++++++++++++--------------
 1 file changed, 44 insertions(+), 25 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a4..df40ef0a5e3 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -119,9 +119,9 @@ static List *sock_paths = NIL;
 #define PQ_RECV_BUFFER_SIZE 8192
 
 static char *PqSendBuffer;
-static int	PqSendBufferSize;	/* Size send buffer */
-static int	PqSendPointer;		/* Next index to store a byte in PqSendBuffer */
-static int	PqSendStart;		/* Next index to send a byte in PqSendBuffer */
+static size_t PqSendBufferSize; /* Size send buffer */
+static size_t PqSendPointer;	/* Next index to store a byte in PqSendBuffer */
+static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
 static char PqRecvBuffer[PQ_RECV_BUFFER_SIZE];
 static int	PqRecvPointer;		/* Next index to read a byte from PqRecvBuffer */
@@ -133,6 +133,7 @@ static int	PqRecvLength;		/* End of data available in PqRecvBuffer */
 static bool PqCommBusy;			/* busy sending data to the client */
 static bool PqCommReadingMsg;	/* in the middle of reading a message */
 
+#define internal_flush()	internal_flush_buffer(PqSendBuffer, &PqSendStart, &PqSendPointer)
 
 /* Internal functions */
 static void socket_comm_reset(void);
@@ -143,8 +144,9 @@ static int	socket_flush_if_writable(void);
 static bool socket_is_send_pending(void);
 static int	socket_putmessage(char msgtype, const char *s, size_t len);
 static void socket_putmessage_noblock(char msgtype, const char *s, size_t len);
-static int	internal_putbytes(const char *s, size_t len);
-static int	internal_flush(void);
+static inline int internal_putbytes(const char *s, size_t len);
+static pg_noinline int internal_flush_buffer(const char *s, size_t *start,
+											 size_t *end);
 
 static int	Lock_AF_UNIX(const char *unixSocketDir, const char *unixSocketPath);
 static int	Setup_AF_UNIX(const char *sock_path);
@@ -1268,11 +1270,9 @@ pq_getmessage(StringInfo s, int maxlen)
 }
 
 
-static int
+static inline int
 internal_putbytes(const char *s, size_t len)
 {
-	size_t		amount;
-
 	while (len > 0)
 	{
 		/* If buffer is full, then flush it out */
@@ -1282,14 +1282,33 @@ internal_putbytes(const char *s, size_t len)
 			if (internal_flush())
 				return EOF;
 		}
-		amount = PqSendBufferSize - PqSendPointer;
-		if (amount > len)
-			amount = len;
-		memcpy(PqSendBuffer + PqSendPointer, s, amount);
-		PqSendPointer += amount;
-		s += amount;
-		len -= amount;
+
+		/*
+		 * If the buffer is empty and data length is larger than the buffer
+		 * size, send it without buffering. Otherwise, put as much data as
+		 * possible into the buffer.
+		 */
+		if (len >= PqSendBufferSize && PqSendStart == PqSendPointer)
+		{
+			size_t		start = 0;
+
+			socket_set_nonblocking(false);
+			if (internal_flush_buffer(s, &start, &len))
+				return EOF;
+		}
+		else
+		{
+			size_t		amount = PqSendBufferSize - PqSendPointer;
+
+			if (amount > len)
+				amount = len;
+			memcpy(PqSendBuffer + PqSendPointer, s, amount);
+			PqSendPointer += amount;
+			s += amount;
+			len -= amount;
+		}
 	}
+
 	return 0;
 }
 
@@ -1315,25 +1334,25 @@ socket_flush(void)
 }
 
 /* --------------------------------
- *		internal_flush - flush pending output
+ *		internal_flush_buffer - flush the given buffer content
  *
  * Returns 0 if OK (meaning everything was sent, or operation would block
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
  */
-static int
-internal_flush(void)
+static pg_noinline int
+internal_flush_buffer(const char *s, size_t *start, size_t *end)
 {
 	static int	last_reported_send_errno = 0;
 
-	char	   *bufptr = PqSendBuffer + PqSendStart;
-	char	   *bufend = PqSendBuffer + PqSendPointer;
+	const char *bufptr = s + *start;
+	const char *bufend = s + *end;
 
 	while (bufptr < bufend)
 	{
 		int			r;
 
-		r = secure_write(MyProcPort, bufptr, bufend - bufptr);
+		r = secure_write(MyProcPort, (char *) bufptr, bufend - bufptr);
 
 		if (r <= 0)
 		{
@@ -1373,7 +1392,7 @@ internal_flush(void)
 			 * flag that'll cause the next CHECK_FOR_INTERRUPTS to terminate
 			 * the connection.
 			 */
-			PqSendStart = PqSendPointer = 0;
+			*start = *end = 0;
 			ClientConnectionLost = 1;
 			InterruptPending = 1;
 			return EOF;
@@ -1381,10 +1400,10 @@ internal_flush(void)
 
 		last_reported_send_errno = 0;	/* reset after any successful send */
 		bufptr += r;
-		PqSendStart += r;
+		*start += r;
 	}
 
-	PqSendStart = PqSendPointer = 0;
+	*start = *end = 0;
 	return 0;
 }
 
@@ -1487,7 +1506,7 @@ static void
 socket_putmessage_noblock(char msgtype, const char *s, size_t len)
 {
 	int			res PG_USED_FOR_ASSERTS_ONLY;
-	int			required;
+	size_t		required;
 
 	/*
 	 * Ensure we have enough space in the output buffer for the message header

base-commit: f956ecd0353b2960f8322b2211142113fe2b6f67
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-07 01:39                     ` Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:42                       ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
  0 siblings, 2 replies; 29+ messages in thread

From: Andres Freund @ 2024-04-07 01:39 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: David Rowley <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

Hi,

On 2024-04-07 00:45:31 +0200, Jelte Fennema-Nio wrote:
> On Sat, 6 Apr 2024 at 22:21, Andres Freund <[email protected]> wrote:
> > The small regression for small results is still kinda visible, I haven't yet
> > tested the patch downthread.
> 
> Thanks a lot for the faster test script, I'm also impatient. I still
> saw the small regression with David his patch. Here's a v6 where I
> think it is now gone. I added inline to internal_put_bytes too. I
> think that helped especially because for two calls to
> internal_put_bytes len is a constant (1 and 4) that is smaller than
> PqSendBufferSize. So for those calls the compiler can now statically
> eliminate the new codepath because "len >= PqSendBufferSize" is known
> to be false at compile time.

Nice.


> Also I incorporated all of Ranier his comments.

Changing the global vars to size_t seems mildly bogus to me. All it's
achieving is to use slightly more memory. It also just seems unrelated to the
change.

Greetings,

Andres Freund






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
@ 2024-04-07 10:04                       ` Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-07 10:04 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: David Rowley <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

On Sun, 7 Apr 2024 at 03:39, Andres Freund <[email protected]> wrote:
> Changing the global vars to size_t seems mildly bogus to me. All it's
> achieving is to use slightly more memory. It also just seems unrelated to the
> change.

I took a closer look at this. I agree that changing PqSendBufferSize
to size_t is unnecessary: given the locations that it is used I see no
risk of overflow anywhere. Changing the type of PqSendPointer and
PqSendStart is needed though, because (as described by Heiki and David
upthread) the argument type of internal_flush_buffer is size_t*. So if
you actually pass int* there, and the sizes are not the same then you
will start writing out of bounds. And because internal_flush_buffer is
introduced in this patch, it is related to this change.

This is what David just committed too.

However, the "required" var actually should be of size_t to avoid
overflow if len is larger than int even without this change. So
attached is a tiny patch that does that.


Attachments:

  [application/octet-stream] v7-0001-Avoid-possible-overflow-in-socket_putmessage_nonb.patch (1009B, ../../CAGECzQQUNpu2NQB01FLEVnxPeYBY1V5eG24z11BTMz9R6J5nyQ@mail.gmail.com/2-v7-0001-Avoid-possible-overflow-in-socket_putmessage_nonb.patch)
  download | inline diff:
From e0320c575f572e7cf6cee8ed4e17016d5ba76901 Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Sun, 7 Apr 2024 10:31:28 +0200
Subject: [PATCH v7 1/2] Avoid possible overflow in socket_putmessage_nonblock

On systems where int consists of fewer bits than size_t there was a
possibility for overflow in socket_putmessage_nonblock, because of an
intermediate cast from size_t to int.
---
 src/backend/libpq/pqcomm.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 6497100a1a4..14186a3e065 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1487,7 +1487,7 @@ static void
 socket_putmessage_noblock(char msgtype, const char *s, size_t len)
 {
 	int			res PG_USED_FOR_ASSERTS_ONLY;
-	int			required;
+	size_t		required;
 
 	/*
 	 * Ensure we have enough space in the output buffer for the message header

base-commit: a97bbe1f1df9eba0b18207c321c67de80b33db16
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-07 12:40                         ` David Rowley <[email protected]>
  2024-04-08 10:42                           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  0 siblings, 1 reply; 29+ messages in thread

From: David Rowley @ 2024-04-07 12:40 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: Andres Freund <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

On Sun, 7 Apr 2024 at 22:05, Jelte Fennema-Nio <[email protected]> wrote:
>
> On Sun, 7 Apr 2024 at 03:39, Andres Freund <[email protected]> wrote:
> > Changing the global vars to size_t seems mildly bogus to me. All it's
> > achieving is to use slightly more memory. It also just seems unrelated to the
> > change.
>
> I took a closer look at this. I agree that changing PqSendBufferSize
> to size_t is unnecessary: given the locations that it is used I see no
> risk of overflow anywhere. Changing the type of PqSendPointer and
> PqSendStart is needed though, because (as described by Heiki and David
> upthread) the argument type of internal_flush_buffer is size_t*. So if
> you actually pass int* there, and the sizes are not the same then you
> will start writing out of bounds. And because internal_flush_buffer is
> introduced in this patch, it is related to this change.
>
> This is what David just committed too.
>
> However, the "required" var actually should be of size_t to avoid
> overflow if len is larger than int even without this change. So
> attached is a tiny patch that does that.

Looking at the code in socket_putmessage_noblock(), I don't understand
why it's ok for PqSendBufferSize to be int but "required" must be
size_t.  There's a line that does "PqSendBufferSize = required;". It
kinda looks like they both should be size_t.  Am I missing something
that you've thought about?

David






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-04-08 10:42                           ` Jelte Fennema-Nio <[email protected]>
  2024-04-08 12:05                             ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
  2024-04-08 12:27                             ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  0 siblings, 2 replies; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-08 10:42 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Andres Freund <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

On Sun, 7 Apr 2024 at 14:41, David Rowley <[email protected]> wrote:
> Looking at the code in socket_putmessage_noblock(), I don't understand
> why it's ok for PqSendBufferSize to be int but "required" must be
> size_t.  There's a line that does "PqSendBufferSize = required;". It
> kinda looks like they both should be size_t.  Am I missing something
> that you've thought about?


You and Ranier are totally right (I missed this assignment). Attached is v8.


Attachments:

  [application/octet-stream] v8-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch (1.3K, ../../CAGECzQRy1DBTkhKgnoFcep5JNk-eGt71Dtf_axc1ihGzXnakFQ@mail.gmail.com/2-v8-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch)
  download | inline diff:
From def294980e01ca412f83fdfcd0b37d27726f2c0f Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Mon, 8 Apr 2024 12:40:01 +0200
Subject: [PATCH v8] Make a few variables size_t in pqcomm.c

This is needed since c4ab7da6061 to avoid overflows in case int is
smaller than size_t.

Reported-By: Ranier Vilela
---
 src/backend/libpq/pqcomm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 2cee49a2085..af43eef2eee 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -119,7 +119,7 @@ static List *sock_paths = NIL;
 #define PQ_RECV_BUFFER_SIZE 8192
 
 static char *PqSendBuffer;
-static int	PqSendBufferSize;	/* Size send buffer */
+static size_t PqSendBufferSize; /* Size send buffer */
 static size_t PqSendPointer;	/* Next index to store a byte in PqSendBuffer */
 static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
@@ -1521,7 +1521,7 @@ static void
 socket_putmessage_noblock(char msgtype, const char *s, size_t len)
 {
 	int			res PG_USED_FOR_ASSERTS_ONLY;
-	int			required;
+	size_t		required;
 
 	/*
 	 * Ensure we have enough space in the output buffer for the message header

base-commit: 422041542f313f23ca66cad26e9b2b99c4d1999a
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-08 10:42                           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-08 12:05                             ` Ranier Vilela <[email protected]>
  1 sibling, 0 replies; 29+ messages in thread

From: Ranier Vilela @ 2024-04-08 12:05 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: David Rowley <[email protected]>; Andres Freund <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Em seg., 8 de abr. de 2024 às 07:42, Jelte Fennema-Nio <[email protected]>
escreveu:

> On Sun, 7 Apr 2024 at 14:41, David Rowley <[email protected]> wrote:
> > Looking at the code in socket_putmessage_noblock(), I don't understand
> > why it's ok for PqSendBufferSize to be int but "required" must be
> > size_t.  There's a line that does "PqSendBufferSize = required;". It
> > kinda looks like they both should be size_t.  Am I missing something
> > that you've thought about?
>
>
> You and Ranier are totally right (I missed this assignment). Attached is
> v8.
>
+1
LGTM.

best regards,
Ranier Vilela


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-08 10:42                           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-08 12:27                             ` Jelte Fennema-Nio <[email protected]>
  2024-04-09 12:02                               ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
  1 sibling, 1 reply; 29+ messages in thread

From: Jelte Fennema-Nio @ 2024-04-08 12:27 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Andres Freund <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>; Ranier Vilela <[email protected]>

On Sun, 7 Apr 2024 at 11:34, David Rowley <[email protected]> wrote:
> That seems to require modifying the following function signatures:
> secure_write(), be_tls_write(), be_gssapi_write().  That's not an area
> I'm familiar with, however.

Attached is a new patchset where 0003 does exactly that. The only
place where we need to cast to non-const is for GSS, but that seems
fine (commit message has more details).

I also added patch 0002, which is a small addition to the function
comment of internal_flush_buffer that seemed useful to me to
differentiate it from internal_flush (feel free to ignore/rewrite).


Attachments:

  [application/octet-stream] v9-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch (1.3K, ../../CAGECzQRQ-i7kEiA7jtjC_VjDUOhykcJUMsy6bj8kwe5+k+9-yw@mail.gmail.com/2-v9-0001-Make-a-few-variables-size_t-in-pqcomm.c.patch)
  download | inline diff:
From def294980e01ca412f83fdfcd0b37d27726f2c0f Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Mon, 8 Apr 2024 12:40:01 +0200
Subject: [PATCH v9 1/3] Make a few variables size_t in pqcomm.c

This is needed since c4ab7da6061 to avoid overflows in case int is
smaller than size_t.

Reported-By: Ranier Vilela
---
 src/backend/libpq/pqcomm.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index 2cee49a2085..af43eef2eee 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -119,7 +119,7 @@ static List *sock_paths = NIL;
 #define PQ_RECV_BUFFER_SIZE 8192
 
 static char *PqSendBuffer;
-static int	PqSendBufferSize;	/* Size send buffer */
+static size_t PqSendBufferSize; /* Size send buffer */
 static size_t PqSendPointer;	/* Next index to store a byte in PqSendBuffer */
 static size_t PqSendStart;		/* Next index to send a byte in PqSendBuffer */
 
@@ -1521,7 +1521,7 @@ static void
 socket_putmessage_noblock(char msgtype, const char *s, size_t len)
 {
 	int			res PG_USED_FOR_ASSERTS_ONLY;
-	int			required;
+	size_t		required;
 
 	/*
 	 * Ensure we have enough space in the output buffer for the message header

base-commit: 422041542f313f23ca66cad26e9b2b99c4d1999a
-- 
2.34.1



  [application/octet-stream] v9-0002-Expand-comment-of-internal_flush_buffer.patch (991B, ../../CAGECzQRQ-i7kEiA7jtjC_VjDUOhykcJUMsy6bj8kwe5+k+9-yw@mail.gmail.com/3-v9-0002-Expand-comment-of-internal_flush_buffer.patch)
  download | inline diff:
From d32640d7500714e755b1b2caeeae06e8a565ce8c Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Mon, 8 Apr 2024 13:27:35 +0200
Subject: [PATCH v9 2/3] Expand comment of internal_flush_buffer

It didn't explain that start and end would be updated, which seems
fairly important.
---
 src/backend/libpq/pqcomm.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index af43eef2eee..a3e80281196 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1351,6 +1351,9 @@ internal_flush(void)
 /* --------------------------------
  *		internal_flush_buffer - flush the given buffer content
  *
+ * Updates start and end to reflect the portion of the buffer that still
+ * needs to be sent.
+ *
  * Returns 0 if OK (meaning everything was sent, or operation would block
  * and the socket is in non-blocking mode), or EOF if trouble.
  * --------------------------------
-- 
2.34.1



  [application/octet-stream] v9-0003-Make-backend-libpq-write-functions-take-const-poi.patch (4.4K, ../../CAGECzQRQ-i7kEiA7jtjC_VjDUOhykcJUMsy6bj8kwe5+k+9-yw@mail.gmail.com/4-v9-0003-Make-backend-libpq-write-functions-take-const-poi.patch)
  download | inline diff:
From e29e44cd9504927458295f694cf4295d073f955e Mon Sep 17 00:00:00 2001
From: Jelte Fennema-Nio <[email protected]>
Date: Mon, 8 Apr 2024 14:10:09 +0200
Subject: [PATCH v9 3/3] Make backend libpq write functions take const pointer

Since c4ab7da6061 we're now passing buffers that shouldn't be modified
to secure_write. This marks all involved functions as taking const
pointers to indicate that that is fine. We were not changing the
contents of this buffer anyway.

The only place where we need to cast it to a non-const pointer is in
be_gssapi_write, but that simply seems an artifact of the GSS API. GSS
has its own buffer type, which is used both for const and non-const
buffers.
---
 src/backend/libpq/be-secure-gssapi.c  | 2 +-
 src/backend/libpq/be-secure-openssl.c | 2 +-
 src/backend/libpq/be-secure.c         | 2 +-
 src/backend/libpq/pqcomm.c            | 2 +-
 src/include/libpq/libpq-be.h          | 4 ++--
 src/include/libpq/libpq.h             | 2 +-
 6 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/src/backend/libpq/be-secure-gssapi.c b/src/backend/libpq/be-secure-gssapi.c
index bc04e78abba..809fe8b959b 100644
--- a/src/backend/libpq/be-secure-gssapi.c
+++ b/src/backend/libpq/be-secure-gssapi.c
@@ -92,7 +92,7 @@ static uint32 PqGSSMaxPktSize;	/* Maximum size we can encrypt and fit the
  * failure if necessary, and then return an errno indicating connection loss.
  */
 ssize_t
-be_gssapi_write(Port *port, void *ptr, size_t len)
+be_gssapi_write(Port *port, const void *ptr, size_t len)
 {
 	OM_uint32	major,
 				minor;
diff --git a/src/backend/libpq/be-secure-openssl.c b/src/backend/libpq/be-secure-openssl.c
index 29c9af1aabf..44ba6d8300c 100644
--- a/src/backend/libpq/be-secure-openssl.c
+++ b/src/backend/libpq/be-secure-openssl.c
@@ -799,7 +799,7 @@ be_tls_read(Port *port, void *ptr, size_t len, int *waitfor)
 }
 
 ssize_t
-be_tls_write(Port *port, void *ptr, size_t len, int *waitfor)
+be_tls_write(Port *port, const void *ptr, size_t len, int *waitfor)
 {
 	ssize_t		n;
 	int			err;
diff --git a/src/backend/libpq/be-secure.c b/src/backend/libpq/be-secure.c
index 1663f36b6b8..edf36139fe3 100644
--- a/src/backend/libpq/be-secure.c
+++ b/src/backend/libpq/be-secure.c
@@ -298,7 +298,7 @@ secure_raw_read(Port *port, void *ptr, size_t len)
  *	Write data to a secure connection.
  */
 ssize_t
-secure_write(Port *port, void *ptr, size_t len)
+secure_write(Port *port, const void *ptr, size_t len)
 {
 	ssize_t		n;
 	int			waitfor;
diff --git a/src/backend/libpq/pqcomm.c b/src/backend/libpq/pqcomm.c
index a3e80281196..7d3df13f532 100644
--- a/src/backend/libpq/pqcomm.c
+++ b/src/backend/libpq/pqcomm.c
@@ -1370,7 +1370,7 @@ internal_flush_buffer(const char *buf, size_t *start, size_t *end)
 	{
 		int			r;
 
-		r = secure_write(MyProcPort, (char *) bufptr, bufend - bufptr);
+		r = secure_write(MyProcPort, bufptr, bufend - bufptr);
 
 		if (r <= 0)
 		{
diff --git a/src/include/libpq/libpq-be.h b/src/include/libpq/libpq-be.h
index 05cb1874c58..7f3e9118800 100644
--- a/src/include/libpq/libpq-be.h
+++ b/src/include/libpq/libpq-be.h
@@ -297,7 +297,7 @@ extern ssize_t be_tls_read(Port *port, void *ptr, size_t len, int *waitfor);
 /*
  * Write data to a secure connection.
  */
-extern ssize_t be_tls_write(Port *port, void *ptr, size_t len, int *waitfor);
+extern ssize_t be_tls_write(Port *port, const void *ptr, size_t len, int *waitfor);
 
 /*
  * Return information about the SSL connection.
@@ -337,7 +337,7 @@ extern bool be_gssapi_get_delegation(Port *port);
 
 /* Read and write to a GSSAPI-encrypted connection. */
 extern ssize_t be_gssapi_read(Port *port, void *ptr, size_t len);
-extern ssize_t be_gssapi_write(Port *port, void *ptr, size_t len);
+extern ssize_t be_gssapi_write(Port *port, const void *ptr, size_t len);
 #endif							/* ENABLE_GSS */
 
 extern PGDLLIMPORT ProtocolVersion FrontendProtocol;
diff --git a/src/include/libpq/libpq.h b/src/include/libpq/libpq.h
index 83e338f604a..d1fab83f8c0 100644
--- a/src/include/libpq/libpq.h
+++ b/src/include/libpq/libpq.h
@@ -105,7 +105,7 @@ extern void secure_destroy(void);
 extern int	secure_open_server(Port *port);
 extern void secure_close(Port *port);
 extern ssize_t secure_read(Port *port, void *ptr, size_t len);
-extern ssize_t secure_write(Port *port, void *ptr, size_t len);
+extern ssize_t secure_write(Port *port, const void *ptr, size_t len);
 extern ssize_t secure_raw_read(Port *port, void *ptr, size_t len);
 extern ssize_t secure_raw_write(Port *port, const void *ptr, size_t len);
 
-- 
2.34.1



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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-08 10:42                           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-08 12:27                             ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
@ 2024-04-09 12:02                               ` Ranier Vilela <[email protected]>
  0 siblings, 0 replies; 29+ messages in thread

From: Ranier Vilela @ 2024-04-09 12:02 UTC (permalink / raw)
  To: Jelte Fennema-Nio <[email protected]>; +Cc: David Rowley <[email protected]>; Andres Freund <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Em seg., 8 de abr. de 2024 às 09:27, Jelte Fennema-Nio <[email protected]>
escreveu:

> On Sun, 7 Apr 2024 at 11:34, David Rowley <[email protected]> wrote:
> > That seems to require modifying the following function signatures:
> > secure_write(), be_tls_write(), be_gssapi_write().  That's not an area
> > I'm familiar with, however.
>
> Attached is a new patchset where 0003 does exactly that. The only
> place where we need to cast to non-const is for GSS, but that seems
> fine (commit message has more details).
>
+1.
Looks ok to me.
The GSS pointer *ptr, is already cast to char * where it is needed,
so the code is already correct.

best regards,
Ranier Vilela


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
  2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
@ 2024-04-07 12:42                       ` Ranier Vilela <[email protected]>
  1 sibling, 0 replies; 29+ messages in thread

From: Ranier Vilela @ 2024-04-07 12:42 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; David Rowley <[email protected]>; Melih Mutlu <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

Em sáb., 6 de abr. de 2024 às 22:39, Andres Freund <[email protected]>
escreveu:

> Hi,
>
> On 2024-04-07 00:45:31 +0200, Jelte Fennema-Nio wrote:
> > On Sat, 6 Apr 2024 at 22:21, Andres Freund <[email protected]> wrote:
> > > The small regression for small results is still kinda visible, I
> haven't yet
> > > tested the patch downthread.
> >
> > Thanks a lot for the faster test script, I'm also impatient. I still
> > saw the small regression with David his patch. Here's a v6 where I
> > think it is now gone. I added inline to internal_put_bytes too. I
> > think that helped especially because for two calls to
> > internal_put_bytes len is a constant (1 and 4) that is smaller than
> > PqSendBufferSize. So for those calls the compiler can now statically
> > eliminate the new codepath because "len >= PqSendBufferSize" is known
> > to be false at compile time.
>
> Nice.
>
>
> > Also I incorporated all of Ranier his comments.
>
> Changing the global vars to size_t seems mildly bogus to me. All it's
> achieving is to use slightly more memory. It also just seems unrelated to
> the
> change.
>
I don't agree with this thought.
Actually size_t uses 4 bytes of memory than int, right.
But mixing up int and size_t is a sure way to write non-portable code.
And the compilers will start showing messages such as " signed/unsigned
mismatch".

The global vars PqSendPointer and PqSendStart were changed in the v5 patch,
so for the sake of style and consistency, I understand that it is better
not to mix the types.

The compiler will promote PqSendBufferSize to size_t in all comparisons.

And finally the correct type to deal with char * variables is size_t.

Best regards,
Ranier Vilela


> Greetings,
>
> Andres Freund
>


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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
@ 2024-04-07 09:33                   ` David Rowley <[email protected]>
  1 sibling, 0 replies; 29+ messages in thread

From: David Rowley @ 2024-04-07 09:33 UTC (permalink / raw)
  To: Andres Freund <[email protected]>; +Cc: Melih Mutlu <[email protected]>; Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

On Sun, 7 Apr 2024 at 08:21, Andres Freund <[email protected]> wrote:
> I added WITH BINARY, SET STORAGE EXTERNAL and tested both unix socket and
> localhost. I also reduced row counts and iteration counts, because I am
> impatient, and I don't think it matters much here. Attached the modified
> version.

Thanks for the script.  I'm able to reproduce the speedup with your script.

I looked over the patch again and ended up making internal_flush an
inline function rather than a macro.  I compared the assembly produced
from each and it's the same with the exception of the label names
being different.

I've now pushed the patch.

One thing that does not seem ideal is having to cast away the
const-ness of the buffer in internal_flush_buffer().  Previously this
wasn't an issue as we always copied the buffer and passed that to
secure_write().  I wonder if it's worth seeing if we can keep this
buffer constant all the way to the socket write.

That seems to require modifying the following function signatures:
secure_write(), be_tls_write(), be_gssapi_write().  That's not an area
I'm familiar with, however.

David






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

* Re: Flushing large data immediately in pqcomm
  2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
  2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
  2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
  2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
@ 2024-04-07 21:56                 ` Melih Mutlu <[email protected]>
  2 siblings, 0 replies; 29+ messages in thread

From: Melih Mutlu @ 2024-04-07 21:56 UTC (permalink / raw)
  To: David Rowley <[email protected]>; +Cc: Jelte Fennema-Nio <[email protected]>; PostgreSQL Hackers <[email protected]>; Andres Freund <[email protected]>; Heikki Linnakangas <[email protected]>; Robert Haas <[email protected]>; Thomas Munro <[email protected]>

David Rowley <[email protected]>, 6 Nis 2024 Cmt, 04:34 tarihinde şunu
yazdı:

> Does anyone else want to try the attached script on the v5 patch to
> see if their numbers are better?
>

I'm seeing the below results with your script on my machine (). I ran it
several times, results were almost similar each time.

master:
Run 100 100 5000000: 1.627905512
Run 1024 10240 200000: 1.603231684
Run 1024 1048576 2000: 2.962812352
Run 1048576 1048576 1000: 2.940766748

v5:
Run 100 100 5000000: 1.611508155
Run 1024 10240 200000: 1.603505596
Run 1024 1048576 2000: 2.727241937
Run 1048576 1048576 1000: 2.721268988


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

end of thread, other threads:[~2024-04-09 12:02 UTC | newest]

Thread overview: 29+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2023-08-09 07:56 [PATCH v4 4/7] Row pattern recognition patch (executor). Tatsuo Ishii <[email protected]>
2024-03-21 00:24 Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-03-21 00:45 ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-03-21 09:44   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-03-21 11:41     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-03-21 09:58 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-03-21 23:45   ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-03-27 11:39     ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-03-27 15:54       ` Re: Flushing large data immediately in pqcomm Robert Haas <[email protected]>
2024-03-28 19:47         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-03-28 19:44       ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-04-04 11:08         ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-04-04 13:34           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-04 14:28             ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[email protected]>
2024-04-06 01:34               ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-04-06 10:16                 ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-06 12:51                   ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-04-06 20:21                 ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
2024-04-06 22:45                   ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-07 01:39                     ` Re: Flushing large data immediately in pqcomm Andres Freund <[email protected]>
2024-04-07 10:04                       ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-07 12:40                         ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-04-08 10:42                           ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-08 12:05                             ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
2024-04-08 12:27                             ` Re: Flushing large data immediately in pqcomm Jelte Fennema-Nio <[email protected]>
2024-04-09 12:02                               ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
2024-04-07 12:42                       ` Re: Flushing large data immediately in pqcomm Ranier Vilela <[email protected]>
2024-04-07 09:33                   ` Re: Flushing large data immediately in pqcomm David Rowley <[email protected]>
2024-04-07 21:56                 ` Re: Flushing large data immediately in pqcomm Melih Mutlu <[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