public inbox for [email protected]  
help / color / mirror / Atom feed
[PATCH 01/10] Allow alternate compression methods for wal_compression
4+ messages / 4 participants
[nested] [flat]

* [PATCH 01/10] Allow alternate compression methods for wal_compression
@ 2021-02-27 04:03  Andrey Borodin <[email protected]>
  0 siblings, 0 replies; 4+ messages in thread

From: Andrey Borodin @ 2021-02-27 04:03 UTC (permalink / raw)

TODO: bump XLOG_PAGE_MAGIC
---
 doc/src/sgml/config.sgml                      | 17 +++++
 src/backend/Makefile                          |  2 +-
 src/backend/access/transam/xlog.c             | 10 +++
 src/backend/access/transam/xloginsert.c       | 52 +++++++++++++--
 src/backend/access/transam/xlogreader.c       | 63 ++++++++++++++++++-
 src/backend/utils/misc/guc.c                  | 11 ++++
 src/backend/utils/misc/postgresql.conf.sample |  1 +
 src/include/access/xlog.h                     |  1 +
 src/include/access/xlog_internal.h            |  8 +++
 src/include/access/xlogreader.h               |  1 +
 src/include/access/xlogrecord.h               |  9 +--
 11 files changed, 163 insertions(+), 12 deletions(-)

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index a218d78bef..7fb2a84626 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -3072,6 +3072,23 @@ include_dir 'conf.d'
       </listitem>
      </varlistentry>
 
+     <varlistentry id="guc-wal-compression-method" xreflabel="wal_compression_method">
+      <term><varname>wal_compressionion_method</varname> (<type>enum</type>)
+      <indexterm>
+       <primary><varname>wal_compression_method</varname> configuration parameter</primary>
+      </indexterm>
+      </term>
+      <listitem>
+       <para>
+        This parameter selects the compression method used to compress WAL when
+        <varname>wal_compression</varname> is enabled.
+        The supported methods are pglz and zlib.
+        The default value is <literal>pglz</literal>.
+        Only superusers can change this setting.
+       </para>
+      </listitem>
+     </varlistentry>
+
      <varlistentry id="guc-wal-init-zero" xreflabel="wal_init_zero">
       <term><varname>wal_init_zero</varname> (<type>boolean</type>)
       <indexterm>
diff --git a/src/backend/Makefile b/src/backend/Makefile
index 0da848b1fd..3af216ddfc 100644
--- a/src/backend/Makefile
+++ b/src/backend/Makefile
@@ -48,7 +48,7 @@ OBJS = \
 LIBS := $(filter-out -lpgport -lpgcommon, $(LIBS)) $(LDAP_LIBS_BE) $(ICU_LIBS)
 
 # The backend doesn't need everything that's in LIBS, however
-LIBS := $(filter-out -lz -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS))
+LIBS := $(filter-out -lreadline -ledit -ltermcap -lncurses -lcurses, $(LIBS))
 
 ifeq ($(with_systemd),yes)
 LIBS += -lsystemd
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index f4d1ce5dea..15da91a8dd 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -99,6 +99,7 @@ bool		EnableHotStandby = false;
 bool		fullPageWrites = true;
 bool		wal_log_hints = false;
 bool		wal_compression = false;
+int			wal_compression_method = WAL_COMPRESSION_PGLZ;
 char	   *wal_consistency_checking_string = NULL;
 bool	   *wal_consistency_checking = NULL;
 bool		wal_init_zero = true;
@@ -180,6 +181,15 @@ const struct config_enum_entry recovery_target_action_options[] = {
 	{NULL, 0, false}
 };
 
+/* Note that due to conditional compilation, offsets within the array are not static */
+const struct config_enum_entry wal_compression_options[] = {
+	{"pglz", WAL_COMPRESSION_PGLZ, false},
+#ifdef  HAVE_LIBZ
+	{"zlib", WAL_COMPRESSION_ZLIB, false},
+#endif
+	{NULL, 0, false}
+};
+
 /*
  * Statistics for current checkpoint are collected in this global struct.
  * Because only the checkpointer or a stand-alone backend can perform
diff --git a/src/backend/access/transam/xloginsert.c b/src/backend/access/transam/xloginsert.c
index 7052dc245e..34e1227381 100644
--- a/src/backend/access/transam/xloginsert.c
+++ b/src/backend/access/transam/xloginsert.c
@@ -33,6 +33,10 @@
 #include "storage/proc.h"
 #include "utils/memutils.h"
 
+#ifdef HAVE_LIBZ
+#include <zlib.h>
+#endif
+
 /* Buffer size required to store a compressed version of backup block image */
 #define PGLZ_MAX_BLCKSZ PGLZ_MAX_OUTPUT(BLCKSZ)
 
@@ -113,7 +117,8 @@ static XLogRecData *XLogRecordAssemble(RmgrId rmid, uint8 info,
 									   XLogRecPtr RedoRecPtr, bool doPageWrites,
 									   XLogRecPtr *fpw_lsn, int *num_fpi);
 static bool XLogCompressBackupBlock(char *page, uint16 hole_offset,
-									uint16 hole_length, char *dest, uint16 *dlen);
+									uint16 hole_length, char *dest,
+									uint16 *dlen, WalCompression compression);
 
 /*
  * Begin constructing a WAL record. This must be called before the
@@ -630,11 +635,12 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
 			 */
 			if (wal_compression)
 			{
+				bimg.compression_method = wal_compression_method;
 				is_compressed =
 					XLogCompressBackupBlock(page, bimg.hole_offset,
 											cbimg.hole_length,
 											regbuf->compressed_page,
-											&compressed_len);
+											&compressed_len, bimg.compression_method);
 			}
 
 			/*
@@ -827,7 +833,7 @@ XLogRecordAssemble(RmgrId rmid, uint8 info,
  */
 static bool
 XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
-						char *dest, uint16 *dlen)
+						char *dest, uint16 *dlen, WalCompression compression)
 {
 	int32		orig_len = BLCKSZ - hole_length;
 	int32		len;
@@ -853,12 +859,48 @@ XLogCompressBackupBlock(char *page, uint16 hole_offset, uint16 hole_length,
 	else
 		source = page;
 
+	switch (compression)
+	{
+	case WAL_COMPRESSION_PGLZ:
+		len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default);
+		break;
+
+#ifdef HAVE_LIBZ
+	case WAL_COMPRESSION_ZLIB:
+		{
+			unsigned long	len_l = PGLZ_MAX_BLCKSZ;
+			int ret;
+			ret = compress2((Bytef*)dest, &len_l, (Bytef*)source, orig_len, 1);
+			if (ret != Z_OK)
+			{
+				// XXX: using an interface other than compress() would allow giving a better error message
+				ereport(ERROR,
+					(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+					 errmsg("failed compressing zlib (%d)", ret)));
+				len_l = -1;
+			}
+			len = len_l;
+			break;
+		}
+#endif
+
+	default:
+		/*
+		 * It should be impossible to get here for unsupported algorithms,
+		 * which cannot be assigned if they're not enabled at compile time.
+		 */
+		ereport(ERROR,
+			(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+			 errmsg("unknown compression method requested: %d(%s)",
+				 compression, wal_compression_name(compression))));
+
+	}
+
 	/*
-	 * We recheck the actual size even if pglz_compress() reports success and
+	 * We recheck the actual size even if compression reports success and
 	 * see if the number of bytes saved by compression is larger than the
 	 * length of extra data needed for the compressed version of block image.
 	 */
-	len = pglz_compress(source, orig_len, dest, PGLZ_strategy_default);
 	if (len >= 0 &&
 		len + extra_bytes < orig_len)
 	{
diff --git a/src/backend/access/transam/xlogreader.c b/src/backend/access/transam/xlogreader.c
index 42738eb940..afca22a26c 100644
--- a/src/backend/access/transam/xlogreader.c
+++ b/src/backend/access/transam/xlogreader.c
@@ -33,6 +33,10 @@
 #include "utils/memutils.h"
 #endif
 
+#ifdef HAVE_LIBZ
+#include <zlib.h>
+#endif
+
 static void report_invalid_record(XLogReaderState *state, const char *fmt,...)
 			pg_attribute_printf(2, 3);
 static bool allocate_recordbuf(XLogReaderState *state, uint32 reclength);
@@ -1286,6 +1290,7 @@ DecodeXLogRecord(XLogReaderState *state, XLogRecord *record, char **errormsg)
 			{
 				COPY_HEADER_FIELD(&blk->bimg_len, sizeof(uint16));
 				COPY_HEADER_FIELD(&blk->hole_offset, sizeof(uint16));
+				COPY_HEADER_FIELD(&blk->compression_method, sizeof(uint8));
 				COPY_HEADER_FIELD(&blk->bimg_info, sizeof(uint8));
 
 				blk->apply_image = ((blk->bimg_info & BKPIMAGE_APPLY) != 0);
@@ -1535,6 +1540,29 @@ XLogRecGetBlockData(XLogReaderState *record, uint8 block_id, Size *len)
 	}
 }
 
+/*
+ * Return a statically allocated string associated with the given compression
+ * method.  This is similar to the guc, but isn't subject to conditional
+ * compilation.
+ */
+const char *
+wal_compression_name(WalCompression compression)
+{
+	/*
+	 * This could index into the guc array, except that it's compiled
+	 * conditionally and unsupported methods are elided.
+	 */
+	switch (compression)
+	{
+		case WAL_COMPRESSION_PGLZ:
+			return "pglz";
+		case WAL_COMPRESSION_ZLIB:
+			return "zlib";
+		default:
+			return "???";
+	}
+}
+
 /*
  * Restore a full-page image from a backup block attached to an XLOG record.
  *
@@ -1558,8 +1586,39 @@ RestoreBlockImage(XLogReaderState *record, uint8 block_id, char *page)
 	if (bkpb->bimg_info & BKPIMAGE_IS_COMPRESSED)
 	{
 		/* If a backup block image is compressed, decompress it */
-		if (pglz_decompress(ptr, bkpb->bimg_len, tmp.data,
-							BLCKSZ - bkpb->hole_length, true) < 0)
+		int32 decomp_result = -1;
+		switch (bkpb->compression_method)
+		{
+		case WAL_COMPRESSION_PGLZ:
+			decomp_result = pglz_decompress(ptr, bkpb->bimg_len, tmp.data,
+							BLCKSZ - bkpb->hole_length, true);
+			break;
+
+#ifdef HAVE_LIBZ
+		case WAL_COMPRESSION_ZLIB:
+		{
+			unsigned long decomp_result_l;
+			decomp_result_l = BLCKSZ - bkpb->hole_length;
+			if (uncompress((Bytef*)tmp.data, &decomp_result_l,
+						(Bytef*)ptr, bkpb->bimg_len) == Z_OK)
+				decomp_result = decomp_result_l;
+			else
+				decomp_result = -1;
+			break;
+		}
+#endif
+
+		default:
+			report_invalid_record(record, "image at %X/%X is compressed with unsupported codec, block %d (%d/%s)",
+								  (uint32) (record->ReadRecPtr >> 32),
+								  (uint32) record->ReadRecPtr,
+								  block_id,
+								  bkpb->compression_method,
+								  wal_compression_name(bkpb->compression_method));
+			return false;
+		}
+
+		if (decomp_result < 0)
 		{
 			report_invalid_record(record, "invalid compressed image at %X/%X, block %d",
 								  LSN_FORMAT_ARGS(record->ReadRecPtr),
diff --git a/src/backend/utils/misc/guc.c b/src/backend/utils/misc/guc.c
index 855076b1fd..8084027465 100644
--- a/src/backend/utils/misc/guc.c
+++ b/src/backend/utils/misc/guc.c
@@ -508,6 +508,7 @@ extern const struct config_enum_entry archive_mode_options[];
 extern const struct config_enum_entry recovery_target_action_options[];
 extern const struct config_enum_entry sync_method_options[];
 extern const struct config_enum_entry dynamic_shared_memory_options[];
+extern const struct config_enum_entry wal_compression_options[];
 
 /*
  * GUC option variables that are exported from this module
@@ -4721,6 +4722,16 @@ static struct config_enum ConfigureNamesEnum[] =
 		NULL, NULL, NULL
 	},
 
+	{
+		{"wal_compression_method", PGC_SIGHUP, WAL_SETTINGS,
+			gettext_noop("Set the method used to compress full page images in the WAL."),
+			NULL
+		},
+		&wal_compression_method,
+		WAL_COMPRESSION_PGLZ, wal_compression_options,
+		NULL, NULL, NULL
+	},
+
 	{
 		{"dynamic_shared_memory_type", PGC_POSTMASTER, RESOURCES_MEM,
 			gettext_noop("Selects the dynamic shared memory implementation used."),
diff --git a/src/backend/utils/misc/postgresql.conf.sample b/src/backend/utils/misc/postgresql.conf.sample
index f46c2dd7a8..ef69a94492 100644
--- a/src/backend/utils/misc/postgresql.conf.sample
+++ b/src/backend/utils/misc/postgresql.conf.sample
@@ -213,6 +213,7 @@
 					#   open_sync
 #full_page_writes = on			# recover from partial page writes
 #wal_compression = off			# enable compression of full-page writes
+#wal_compression_method = pglz		# pglz, zlib
 #wal_log_hints = off			# also do full page writes of non-critical updates
 					# (change requires restart)
 #wal_init_zero = on			# zero-fill new WAL files
diff --git a/src/include/access/xlog.h b/src/include/access/xlog.h
index 6d384d3ce6..fa2e5c611f 100644
--- a/src/include/access/xlog.h
+++ b/src/include/access/xlog.h
@@ -117,6 +117,7 @@ extern bool EnableHotStandby;
 extern bool fullPageWrites;
 extern bool wal_log_hints;
 extern bool wal_compression;
+extern int	wal_compression_method;
 extern bool wal_init_zero;
 extern bool wal_recycle;
 extern bool *wal_consistency_checking;
diff --git a/src/include/access/xlog_internal.h b/src/include/access/xlog_internal.h
index b23e286406..d653839b97 100644
--- a/src/include/access/xlog_internal.h
+++ b/src/include/access/xlog_internal.h
@@ -324,4 +324,12 @@ extern bool InArchiveRecovery;
 extern bool StandbyMode;
 extern char *recoveryRestoreCommand;
 
+typedef enum WalCompression
+{
+	WAL_COMPRESSION_PGLZ,
+	WAL_COMPRESSION_ZLIB,
+} WalCompression;
+
+extern const char *wal_compression_name(WalCompression compression);
+
 #endif							/* XLOG_INTERNAL_H */
diff --git a/src/include/access/xlogreader.h b/src/include/access/xlogreader.h
index 21d200d3df..3d19c315d7 100644
--- a/src/include/access/xlogreader.h
+++ b/src/include/access/xlogreader.h
@@ -133,6 +133,7 @@ typedef struct
 	bool		apply_image;	/* has image that should be restored */
 	char	   *bkp_image;
 	uint16		hole_offset;
+	uint8		compression_method;
 	uint16		hole_length;
 	uint16		bimg_len;
 	uint8		bimg_info;
diff --git a/src/include/access/xlogrecord.h b/src/include/access/xlogrecord.h
index 80c92a2498..0d4c212f15 100644
--- a/src/include/access/xlogrecord.h
+++ b/src/include/access/xlogrecord.h
@@ -114,7 +114,7 @@ typedef struct XLogRecordBlockHeader
  * present is (BLCKSZ - <length of "hole" bytes>).
  *
  * Additionally, when wal_compression is enabled, we will try to compress full
- * page images using the PGLZ compression algorithm, after removing the "hole".
+ * page images, after removing the "hole".
  * This can reduce the WAL volume, but at some extra cost of CPU spent
  * on the compression during WAL logging. In this case, since the "hole"
  * length cannot be calculated by subtracting the number of page image bytes
@@ -129,9 +129,10 @@ typedef struct XLogRecordBlockHeader
  */
 typedef struct XLogRecordBlockImageHeader
 {
-	uint16		length;			/* number of page image bytes */
-	uint16		hole_offset;	/* number of bytes before "hole" */
-	uint8		bimg_info;		/* flag bits, see below */
+	uint16		length;				/* number of page image bytes */
+	uint16		hole_offset;		/* number of bytes before "hole" */
+	uint8		compression_method; /* compression method used for image */
+	uint8		bimg_info;			/* flag bits, see below */
 
 	/*
 	 * If BKPIMAGE_HAS_HOLE and BKPIMAGE_IS_COMPRESSED, an
-- 
2.17.0


--XsQoSWH+UP9D9v3l
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
 filename="0002-Run-011_crash_recovery.pl-with-wal_level-minimal.patch"



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

* optimize hashjoin
@ 2024-08-21 01:49  bucoo <[email protected]>
  0 siblings, 1 reply; 4+ messages in thread

From: bucoo @ 2024-08-21 01:49 UTC (permalink / raw)
  To: pgsql-hackers; +Cc: 'Tom Lane' <[email protected]>; 'Robert Haas' <[email protected]>

Avoid unnecessary form and deform tuple.

In the TPCH test, HashJoin speed up to ~2x.


diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 61480733a1..2dad0c8a55 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1627,6 +1627,23 @@ ExecHashTableInsert(HashJoinTable hashtable,
 {
 	bool		shouldFree;
 	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
+
+	if (shouldFree)
+		heap_free_minimal_tuple(tuple);
+}
+
+/*
+ * ExecHashTableInsert
+ *		insert a tuple into the hash table depending on the hash value
+ *		it may just go to a temp file for later batches
+ */
+void
+ExecHashTableInsertTuple(HashJoinTable hashtable,
+						 MinimalTuple tuple,
+						 uint32 hashvalue)
+{
 	int			bucketno;
 	int			batchno;
 
@@ -1701,9 +1718,6 @@ ExecHashTableInsert(HashJoinTable hashtable,
 							  &hashtable->innerBatchFile[batchno],
 							  hashtable);
 	}
-
-	if (shouldFree)
-		heap_free_minimal_tuple(tuple);
 }
 
 /*
@@ -1777,12 +1791,10 @@ retry:
  * tuples that belong in the current batch once growth has been disabled.
  */
 void
-ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
-										TupleTableSlot *slot,
-										uint32 hashvalue)
+ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+											 MinimalTuple tuple,
+											 uint32 hashvalue)
 {
-	bool		shouldFree;
-	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
 	HashJoinTuple hashTuple;
 	dsa_pointer shared;
 	int			batchno;
@@ -1798,6 +1810,21 @@ ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 	HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));
 	ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],
 							  hashTuple, shared);
+}
+
+/*
+ * like ExecParallelHashTableInsertCurrentBatchTuple,
+ * but this function accept a TupleTableSlot
+ */
+void
+ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
+										TupleTableSlot *slot,
+										uint32 hashvalue)
+{
+	bool		shouldFree;
+	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecParallelHashTableInsertCurrentBatchTuple(hashtable, tuple, hashvalue);
 
 	if (shouldFree)
 		heap_free_minimal_tuple(tuple);
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5f4073eabd..002098f129 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -194,10 +194,10 @@ static TupleTableSlot *ExecHashJoinOuterGetTuple(PlanState *outerNode,
 static TupleTableSlot *ExecParallelHashJoinOuterGetTuple(PlanState *outerNode,
 														 HashJoinState *hjstate,
 														 uint32 *hashvalue);
-static TupleTableSlot *ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
-												 BufFile *file,
-												 uint32 *hashvalue,
-												 TupleTableSlot *tupleSlot);
+static MinimalTuple ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
+											  BufFile *file,
+											  uint32 *hashvalue,
+											  StringInfo buf);
 static bool ExecHashJoinNewBatch(HashJoinState *hjstate);
 static bool ExecParallelHashJoinNewBatch(HashJoinState *hjstate);
 static void ExecParallelHashJoinPartitionOuter(HashJoinState *hjstate);
@@ -831,6 +831,7 @@ ExecInitHashJoin(HashJoin *node, EState *estate, int eflags)
 	 */
 	hjstate->hj_HashTable = NULL;
 	hjstate->hj_FirstOuterTupleSlot = NULL;
+	hjstate->hj_outerTupleBuffer = NULL;
 
 	hjstate->hj_CurHashValue = 0;
 	hjstate->hj_CurBucketNo = 0;
@@ -936,6 +937,7 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 	}
 	else if (curbatch < hashtable->nbatch)
 	{
+		MinimalTuple mtup;
 		BufFile    *file = hashtable->outerBatchFile[curbatch];
 
 		/*
@@ -945,12 +947,23 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 		if (file == NULL)
 			return NULL;
 
-		slot = ExecHashJoinGetSavedTuple(hjstate,
+		if (unlikely(hjstate->hj_outerTupleBuffer == NULL))
+		{
+			MemoryContext oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(hjstate));
+			hjstate->hj_outerTupleBuffer = makeStringInfo();
+			MemoryContextSwitchTo(oldcontext);
+		}
+
+		mtup = ExecHashJoinGetSavedTuple(hjstate,
 										 file,
 										 hashvalue,
-										 hjstate->hj_OuterTupleSlot);
-		if (!TupIsNull(slot))
+										 hjstate->hj_outerTupleBuffer);
+		if (likely(mtup != NULL))
+		{
+			slot = hjstate->hj_OuterTupleSlot;
+			ExecForceStoreMinimalTuple(mtup, slot, false);
 			return slot;
+		}
 	}
 
 	/* End of this batch */
@@ -1034,7 +1047,6 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 	int			nbatch;
 	int			curbatch;
 	BufFile    *innerFile;
-	TupleTableSlot *slot;
 	uint32		hashvalue;
 
 	nbatch = hashtable->nbatch;
@@ -1125,21 +1137,25 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 
 	if (innerFile != NULL)
 	{
+		StringInfoData buf;
+		MinimalTuple tuple;
+
 		if (BufFileSeek(innerFile, 0, 0, SEEK_SET))
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not rewind hash-join temporary file")));
 
-		while ((slot = ExecHashJoinGetSavedTuple(hjstate,
-												 innerFile,
-												 &hashvalue,
-												 hjstate->hj_HashTupleSlot)))
+		initStringInfo(&buf);
+		while ((tuple = ExecHashJoinGetSavedTuple(hjstate,
+												  innerFile,
+												  &hashvalue,
+												  &buf)))
 		{
 			/*
 			 * NOTE: some tuples may be sent to future batches.  Also, it is
 			 * possible for hashtable->nbatch to be increased here!
 			 */
-			ExecHashTableInsert(hashtable, slot, hashvalue);
+			ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
 		}
 
 		/*
@@ -1148,6 +1164,7 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 		 */
 		BufFileClose(innerFile);
 		hashtable->innerBatchFile[curbatch] = NULL;
+		pfree(buf.data);
 	}
 
 	/*
@@ -1198,7 +1215,6 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 	{
 		uint32		hashvalue;
 		MinimalTuple tuple;
-		TupleTableSlot *slot;
 
 		if (!hashtable->batches[batchno].done)
 		{
@@ -1230,12 +1246,9 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 					while ((tuple = sts_parallel_scan_next(inner_tuples,
 														   &hashvalue)))
 					{
-						ExecForceStoreMinimalTuple(tuple,
-												   hjstate->hj_HashTupleSlot,
-												   false);
-						slot = hjstate->hj_HashTupleSlot;
-						ExecParallelHashTableInsertCurrentBatch(hashtable, slot,
-																hashvalue);
+						ExecParallelHashTableInsertCurrentBatchTuple(hashtable,
+																	 tuple,
+																	 hashvalue);
 					}
 					sts_end_parallel_scan(inner_tuples);
 					BarrierArriveAndWait(batch_barrier,
@@ -1349,14 +1362,14 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
  * ExecHashJoinGetSavedTuple
  *		read the next tuple from a batch file.  Return NULL if no more.
  *
- * On success, *hashvalue is set to the tuple's hash value, and the tuple
- * itself is stored in the given slot.
+ * On success, *hashvalue is set to the tuple's hash value, and return
+ * the tuple(stored in the given buf) itself.
  */
-static TupleTableSlot *
+static MinimalTuple
 ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 						  BufFile *file,
 						  uint32 *hashvalue,
-						  TupleTableSlot *tupleSlot)
+						  StringInfo buf)
 {
 	uint32		header[2];
 	size_t		nread;
@@ -1375,19 +1388,19 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 	 * cheating.
 	 */
 	nread = BufFileReadMaybeEOF(file, header, sizeof(header), true);
-	if (nread == 0)				/* end of file */
-	{
-		ExecClearTuple(tupleSlot);
+	if (unlikely(nread == 0))				/* end of file */
 		return NULL;
-	}
+
+	enlargeStringInfo(buf, header[1]);
 	*hashvalue = header[0];
-	tuple = (MinimalTuple) palloc(header[1]);
+	buf->len = header[1];
+	tuple = (MinimalTuple) buf->data;
 	tuple->t_len = header[1];
 	BufFileReadExact(file,
 					 (char *) tuple + sizeof(uint32),
 					 header[1] - sizeof(uint32));
-	ExecForceStoreMinimalTuple(tuple, tupleSlot, true);
-	return tupleSlot;
+
+	return tuple;
 }
 
 
diff --git a/src/include/executor/nodeHash.h b/src/include/executor/nodeHash.h
index a95911c2fe..543f64cdf7 100644
--- a/src/include/executor/nodeHash.h
+++ b/src/include/executor/nodeHash.h
@@ -37,12 +37,18 @@ extern void ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable,
 extern void ExecHashTableInsert(HashJoinTable hashtable,
 								TupleTableSlot *slot,
 								uint32 hashvalue);
+extern void ExecHashTableInsertTuple(HashJoinTable hashtable,
+									 MinimalTuple tuple,
+									 uint32 hashvalue);
 extern void ExecParallelHashTableInsert(HashJoinTable hashtable,
 										TupleTableSlot *slot,
 										uint32 hashvalue);
 extern void ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 													TupleTableSlot *slot,
 													uint32 hashvalue);
+extern void ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+														 MinimalTuple tuple,
+														 uint32 hashvalue);
 extern bool ExecHashGetHashValue(HashJoinTable hashtable,
 								 ExprContext *econtext,
 								 List *hashkeys,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b62c96f206..e5e3a0a155 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2200,6 +2200,7 @@ typedef struct HashJoinState
 	TupleTableSlot *hj_NullOuterTupleSlot;
 	TupleTableSlot *hj_NullInnerTupleSlot;
 	TupleTableSlot *hj_FirstOuterTupleSlot;
+	StringInfo	hj_outerTupleBuffer;
 	int			hj_JoinState;
 	bool		hj_MatchedOuter;
 	bool		hj_OuterNotEmpty;


Attachments:

  [text/plain] optimize-hashjoin.patch (9.3K, ../../[email protected]/3-optimize-hashjoin.patch)
  download | inline diff:
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 61480733a1..2dad0c8a55 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1627,6 +1627,23 @@ ExecHashTableInsert(HashJoinTable hashtable,
 {
 	bool		shouldFree;
 	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
+
+	if (shouldFree)
+		heap_free_minimal_tuple(tuple);
+}
+
+/*
+ * ExecHashTableInsert
+ *		insert a tuple into the hash table depending on the hash value
+ *		it may just go to a temp file for later batches
+ */
+void
+ExecHashTableInsertTuple(HashJoinTable hashtable,
+						 MinimalTuple tuple,
+						 uint32 hashvalue)
+{
 	int			bucketno;
 	int			batchno;
 
@@ -1701,9 +1718,6 @@ ExecHashTableInsert(HashJoinTable hashtable,
 							  &hashtable->innerBatchFile[batchno],
 							  hashtable);
 	}
-
-	if (shouldFree)
-		heap_free_minimal_tuple(tuple);
 }
 
 /*
@@ -1777,12 +1791,10 @@ retry:
  * tuples that belong in the current batch once growth has been disabled.
  */
 void
-ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
-										TupleTableSlot *slot,
-										uint32 hashvalue)
+ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+											 MinimalTuple tuple,
+											 uint32 hashvalue)
 {
-	bool		shouldFree;
-	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
 	HashJoinTuple hashTuple;
 	dsa_pointer shared;
 	int			batchno;
@@ -1798,6 +1810,21 @@ ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 	HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));
 	ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],
 							  hashTuple, shared);
+}
+
+/*
+ * like ExecParallelHashTableInsertCurrentBatchTuple,
+ * but this function accept a TupleTableSlot
+ */
+void
+ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
+										TupleTableSlot *slot,
+										uint32 hashvalue)
+{
+	bool		shouldFree;
+	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecParallelHashTableInsertCurrentBatchTuple(hashtable, tuple, hashvalue);
 
 	if (shouldFree)
 		heap_free_minimal_tuple(tuple);
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 5f4073eabd..002098f129 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -194,10 +194,10 @@ static TupleTableSlot *ExecHashJoinOuterGetTuple(PlanState *outerNode,
 static TupleTableSlot *ExecParallelHashJoinOuterGetTuple(PlanState *outerNode,
 														 HashJoinState *hjstate,
 														 uint32 *hashvalue);
-static TupleTableSlot *ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
-												 BufFile *file,
-												 uint32 *hashvalue,
-												 TupleTableSlot *tupleSlot);
+static MinimalTuple ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
+											  BufFile *file,
+											  uint32 *hashvalue,
+											  StringInfo buf);
 static bool ExecHashJoinNewBatch(HashJoinState *hjstate);
 static bool ExecParallelHashJoinNewBatch(HashJoinState *hjstate);
 static void ExecParallelHashJoinPartitionOuter(HashJoinState *hjstate);
@@ -831,6 +831,7 @@ ExecInitHashJoin(HashJoin *node, EState *estate, int eflags)
 	 */
 	hjstate->hj_HashTable = NULL;
 	hjstate->hj_FirstOuterTupleSlot = NULL;
+	hjstate->hj_outerTupleBuffer = NULL;
 
 	hjstate->hj_CurHashValue = 0;
 	hjstate->hj_CurBucketNo = 0;
@@ -936,6 +937,7 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 	}
 	else if (curbatch < hashtable->nbatch)
 	{
+		MinimalTuple mtup;
 		BufFile    *file = hashtable->outerBatchFile[curbatch];
 
 		/*
@@ -945,12 +947,23 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 		if (file == NULL)
 			return NULL;
 
-		slot = ExecHashJoinGetSavedTuple(hjstate,
+		if (unlikely(hjstate->hj_outerTupleBuffer == NULL))
+		{
+			MemoryContext oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(hjstate));
+			hjstate->hj_outerTupleBuffer = makeStringInfo();
+			MemoryContextSwitchTo(oldcontext);
+		}
+
+		mtup = ExecHashJoinGetSavedTuple(hjstate,
 										 file,
 										 hashvalue,
-										 hjstate->hj_OuterTupleSlot);
-		if (!TupIsNull(slot))
+										 hjstate->hj_outerTupleBuffer);
+		if (likely(mtup != NULL))
+		{
+			slot = hjstate->hj_OuterTupleSlot;
+			ExecForceStoreMinimalTuple(mtup, slot, false);
 			return slot;
+		}
 	}
 
 	/* End of this batch */
@@ -1034,7 +1047,6 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 	int			nbatch;
 	int			curbatch;
 	BufFile    *innerFile;
-	TupleTableSlot *slot;
 	uint32		hashvalue;
 
 	nbatch = hashtable->nbatch;
@@ -1125,21 +1137,25 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 
 	if (innerFile != NULL)
 	{
+		StringInfoData buf;
+		MinimalTuple tuple;
+
 		if (BufFileSeek(innerFile, 0, 0, SEEK_SET))
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not rewind hash-join temporary file")));
 
-		while ((slot = ExecHashJoinGetSavedTuple(hjstate,
-												 innerFile,
-												 &hashvalue,
-												 hjstate->hj_HashTupleSlot)))
+		initStringInfo(&buf);
+		while ((tuple = ExecHashJoinGetSavedTuple(hjstate,
+												  innerFile,
+												  &hashvalue,
+												  &buf)))
 		{
 			/*
 			 * NOTE: some tuples may be sent to future batches.  Also, it is
 			 * possible for hashtable->nbatch to be increased here!
 			 */
-			ExecHashTableInsert(hashtable, slot, hashvalue);
+			ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
 		}
 
 		/*
@@ -1148,6 +1164,7 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 		 */
 		BufFileClose(innerFile);
 		hashtable->innerBatchFile[curbatch] = NULL;
+		pfree(buf.data);
 	}
 
 	/*
@@ -1198,7 +1215,6 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 	{
 		uint32		hashvalue;
 		MinimalTuple tuple;
-		TupleTableSlot *slot;
 
 		if (!hashtable->batches[batchno].done)
 		{
@@ -1230,12 +1246,9 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 					while ((tuple = sts_parallel_scan_next(inner_tuples,
 														   &hashvalue)))
 					{
-						ExecForceStoreMinimalTuple(tuple,
-												   hjstate->hj_HashTupleSlot,
-												   false);
-						slot = hjstate->hj_HashTupleSlot;
-						ExecParallelHashTableInsertCurrentBatch(hashtable, slot,
-																hashvalue);
+						ExecParallelHashTableInsertCurrentBatchTuple(hashtable,
+																	 tuple,
+																	 hashvalue);
 					}
 					sts_end_parallel_scan(inner_tuples);
 					BarrierArriveAndWait(batch_barrier,
@@ -1349,14 +1362,14 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
  * ExecHashJoinGetSavedTuple
  *		read the next tuple from a batch file.  Return NULL if no more.
  *
- * On success, *hashvalue is set to the tuple's hash value, and the tuple
- * itself is stored in the given slot.
+ * On success, *hashvalue is set to the tuple's hash value, and return
+ * the tuple(stored in the given buf) itself.
  */
-static TupleTableSlot *
+static MinimalTuple
 ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 						  BufFile *file,
 						  uint32 *hashvalue,
-						  TupleTableSlot *tupleSlot)
+						  StringInfo buf)
 {
 	uint32		header[2];
 	size_t		nread;
@@ -1375,19 +1388,19 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 	 * cheating.
 	 */
 	nread = BufFileReadMaybeEOF(file, header, sizeof(header), true);
-	if (nread == 0)				/* end of file */
-	{
-		ExecClearTuple(tupleSlot);
+	if (unlikely(nread == 0))				/* end of file */
 		return NULL;
-	}
+
+	enlargeStringInfo(buf, header[1]);
 	*hashvalue = header[0];
-	tuple = (MinimalTuple) palloc(header[1]);
+	buf->len = header[1];
+	tuple = (MinimalTuple) buf->data;
 	tuple->t_len = header[1];
 	BufFileReadExact(file,
 					 (char *) tuple + sizeof(uint32),
 					 header[1] - sizeof(uint32));
-	ExecForceStoreMinimalTuple(tuple, tupleSlot, true);
-	return tupleSlot;
+
+	return tuple;
 }
 
 
diff --git a/src/include/executor/nodeHash.h b/src/include/executor/nodeHash.h
index a95911c2fe..543f64cdf7 100644
--- a/src/include/executor/nodeHash.h
+++ b/src/include/executor/nodeHash.h
@@ -37,12 +37,18 @@ extern void ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable,
 extern void ExecHashTableInsert(HashJoinTable hashtable,
 								TupleTableSlot *slot,
 								uint32 hashvalue);
+extern void ExecHashTableInsertTuple(HashJoinTable hashtable,
+									 MinimalTuple tuple,
+									 uint32 hashvalue);
 extern void ExecParallelHashTableInsert(HashJoinTable hashtable,
 										TupleTableSlot *slot,
 										uint32 hashvalue);
 extern void ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 													TupleTableSlot *slot,
 													uint32 hashvalue);
+extern void ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+														 MinimalTuple tuple,
+														 uint32 hashvalue);
 extern bool ExecHashGetHashValue(HashJoinTable hashtable,
 								 ExprContext *econtext,
 								 List *hashkeys,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index b62c96f206..e5e3a0a155 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2200,6 +2200,7 @@ typedef struct HashJoinState
 	TupleTableSlot *hj_NullOuterTupleSlot;
 	TupleTableSlot *hj_NullInnerTupleSlot;
 	TupleTableSlot *hj_FirstOuterTupleSlot;
+	StringInfo	hj_outerTupleBuffer;
 	int			hj_JoinState;
 	bool		hj_MatchedOuter;
 	bool		hj_OuterNotEmpty;


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

* Re: optimize hashjoin
@ 2024-08-21 16:31  Tomas Vondra <[email protected]>
  parent: bucoo <[email protected]>
  0 siblings, 1 reply; 4+ messages in thread

From: Tomas Vondra @ 2024-08-21 16:31 UTC (permalink / raw)
  To: bucoo <[email protected]>; pgsql-hackers; +Cc: 'Tom Lane' <[email protected]>; 'Robert Haas' <[email protected]>

Hi,

On 8/21/24 03:49, bucoo wrote:
> Avoid unnecessary form and deform tuple.
> 

Thanks for the patch. Generally speaking, it's usually a good idea to
briefly explain what the patch does, why is that beneficial, in what
circumstances, etc. It's sometimes not quite obvious from the patch
itself (even if the patch is simple). Plus it really helps new
contributors who want to review the patch, but even for experienced
people it's a huge time saver ...

Anyway, I took a look and the basic idea is simple - when shuffling
tuples between batches in a hash join, we're currently deforming the
tuple (->slot) we just read from a batch, only to immediately form it
(slot->) again and write it to the "correct" batch.

I think the idea to skip this unnecessary deform/form step is sound, and
I don't see any particular argument against doing that. The code looks
reasonable too, I think.

A couple minor comments:

0) The patch does not apply anymore, thanks to David committing a patch
yesterday. Attached is a patch rebased on top of current master.

1) Wouldn't it be easier (and just as efficient) to use slots with
TTSOpsMinimalTuple, i.e. backed by a minimal tuple?

2) I find the naming of the new functions a bit confusing. We now have
the "original" functions working with slots, and then also functions
with "Tuple" working with tuples. Seems asymmetric.

3) The code in ExecHashJoinOuterGetTuple is hard to understand, it'd
very much benefit from some comments. I'm a bit unsure if the likely()
and unlikely() hints really help.

4) Is the hj_outerTupleBuffer buffer really needed / effective? I'd bet
just using palloc() will work just as well, thanks to the AllocSet
caching etc.

5) You might want to add the patch to the 2024-09 CF, to keep track of
it: https://commitfest.postgresql.org/49/

> In the TPCH test, HashJoin speed up to ~2x.
> 
Can you provide more information about the benchmark you did? What
hardware, what scale, PostgreSQL configuration, which of the 22 queries
are improved, etc.

I ran TPC-H with 1GB and 10GB scales on two machines, and I see pretty
much no difference compared to master. However, it occurred to me the
patch only ever helps if we increase the number of batches during
execution, in which case we need to move tuples to the right batch.

Perhaps that's not happening in my testing, but it was happening in your
runs? Which just means we really need to know more about how you did the
testing.


regards

-- 
Tomas Vondra

Attachments:

  [text/x-patch] optimize-hashjoin-rebased-tomas.patch (9.7K, ../../[email protected]/2-optimize-hashjoin-rebased-tomas.patch)
  download | inline diff:
diff --git a/src/backend/executor/nodeHash.c b/src/backend/executor/nodeHash.c
index 570a90ebe15..6f5cd16aa2b 100644
--- a/src/backend/executor/nodeHash.c
+++ b/src/backend/executor/nodeHash.c
@@ -1611,6 +1611,23 @@ ExecHashTableInsert(HashJoinTable hashtable,
 {
 	bool		shouldFree;
 	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
+
+	if (shouldFree)
+		heap_free_minimal_tuple(tuple);
+}
+
+/*
+ * ExecHashTableInsert
+ *		insert a tuple into the hash table depending on the hash value
+ *		it may just go to a temp file for later batches
+ */
+void
+ExecHashTableInsertTuple(HashJoinTable hashtable,
+						 MinimalTuple tuple,
+						 uint32 hashvalue)
+{
 	int			bucketno;
 	int			batchno;
 
@@ -1685,9 +1702,6 @@ ExecHashTableInsert(HashJoinTable hashtable,
 							  &hashtable->innerBatchFile[batchno],
 							  hashtable);
 	}
-
-	if (shouldFree)
-		heap_free_minimal_tuple(tuple);
 }
 
 /*
@@ -1761,12 +1775,10 @@ retry:
  * tuples that belong in the current batch once growth has been disabled.
  */
 void
-ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
-										TupleTableSlot *slot,
-										uint32 hashvalue)
+ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+											 MinimalTuple tuple,
+											 uint32 hashvalue)
 {
-	bool		shouldFree;
-	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
 	HashJoinTuple hashTuple;
 	dsa_pointer shared;
 	int			batchno;
@@ -1782,6 +1794,21 @@ ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 	HeapTupleHeaderClearMatch(HJTUPLE_MINTUPLE(hashTuple));
 	ExecParallelHashPushTuple(&hashtable->buckets.shared[bucketno],
 							  hashTuple, shared);
+}
+
+/*
+ * like ExecParallelHashTableInsertCurrentBatchTuple,
+ * but this function accept a TupleTableSlot
+ */
+void
+ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
+										TupleTableSlot *slot,
+										uint32 hashvalue)
+{
+	bool		shouldFree;
+	MinimalTuple tuple = ExecFetchSlotMinimalTuple(slot, &shouldFree);
+
+	ExecParallelHashTableInsertCurrentBatchTuple(hashtable, tuple, hashvalue);
 
 	if (shouldFree)
 		heap_free_minimal_tuple(tuple);
diff --git a/src/backend/executor/nodeHashjoin.c b/src/backend/executor/nodeHashjoin.c
index 2f7170604d6..d52e4e14366 100644
--- a/src/backend/executor/nodeHashjoin.c
+++ b/src/backend/executor/nodeHashjoin.c
@@ -195,10 +195,10 @@ static TupleTableSlot *ExecHashJoinOuterGetTuple(PlanState *outerNode,
 static TupleTableSlot *ExecParallelHashJoinOuterGetTuple(PlanState *outerNode,
 														 HashJoinState *hjstate,
 														 uint32 *hashvalue);
-static TupleTableSlot *ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
-												 BufFile *file,
-												 uint32 *hashvalue,
-												 TupleTableSlot *tupleSlot);
+static MinimalTuple ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
+											  BufFile *file,
+											  uint32 *hashvalue,
+											  StringInfo buf);
 static bool ExecHashJoinNewBatch(HashJoinState *hjstate);
 static bool ExecParallelHashJoinNewBatch(HashJoinState *hjstate);
 static void ExecParallelHashJoinPartitionOuter(HashJoinState *hjstate);
@@ -925,6 +925,7 @@ ExecInitHashJoin(HashJoin *node, EState *estate, int eflags)
 	 */
 	hjstate->hj_HashTable = NULL;
 	hjstate->hj_FirstOuterTupleSlot = NULL;
+	hjstate->hj_outerTupleBuffer = NULL;
 
 	hjstate->hj_CurHashValue = 0;
 	hjstate->hj_CurBucketNo = 0;
@@ -1030,6 +1031,7 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 	}
 	else if (curbatch < hashtable->nbatch)
 	{
+		MinimalTuple mtup;
 		BufFile    *file = hashtable->outerBatchFile[curbatch];
 
 		/*
@@ -1039,12 +1041,23 @@ ExecHashJoinOuterGetTuple(PlanState *outerNode,
 		if (file == NULL)
 			return NULL;
 
-		slot = ExecHashJoinGetSavedTuple(hjstate,
+		if (unlikely(hjstate->hj_outerTupleBuffer == NULL))
+		{
+			MemoryContext oldcontext = MemoryContextSwitchTo(GetMemoryChunkContext(hjstate));
+			hjstate->hj_outerTupleBuffer = makeStringInfo();
+			MemoryContextSwitchTo(oldcontext);
+		}
+
+		mtup = ExecHashJoinGetSavedTuple(hjstate,
 										 file,
 										 hashvalue,
-										 hjstate->hj_OuterTupleSlot);
-		if (!TupIsNull(slot))
+										 hjstate->hj_outerTupleBuffer);
+		if (likely(mtup != NULL))
+		{
+			slot = hjstate->hj_OuterTupleSlot;
+			ExecForceStoreMinimalTuple(mtup, slot, false);
 			return slot;
+		}
 	}
 
 	/* End of this batch */
@@ -1133,7 +1146,6 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 	int			nbatch;
 	int			curbatch;
 	BufFile    *innerFile;
-	TupleTableSlot *slot;
 	uint32		hashvalue;
 
 	nbatch = hashtable->nbatch;
@@ -1224,21 +1236,25 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 
 	if (innerFile != NULL)
 	{
+		StringInfoData buf;
+		MinimalTuple tuple;
+
 		if (BufFileSeek(innerFile, 0, 0, SEEK_SET))
 			ereport(ERROR,
 					(errcode_for_file_access(),
 					 errmsg("could not rewind hash-join temporary file")));
 
-		while ((slot = ExecHashJoinGetSavedTuple(hjstate,
-												 innerFile,
-												 &hashvalue,
-												 hjstate->hj_HashTupleSlot)))
+		initStringInfo(&buf);
+		while ((tuple = ExecHashJoinGetSavedTuple(hjstate,
+												  innerFile,
+												  &hashvalue,
+												  &buf)))
 		{
 			/*
 			 * NOTE: some tuples may be sent to future batches.  Also, it is
 			 * possible for hashtable->nbatch to be increased here!
 			 */
-			ExecHashTableInsert(hashtable, slot, hashvalue);
+			ExecHashTableInsertTuple(hashtable, tuple, hashvalue);
 		}
 
 		/*
@@ -1247,6 +1263,7 @@ ExecHashJoinNewBatch(HashJoinState *hjstate)
 		 */
 		BufFileClose(innerFile);
 		hashtable->innerBatchFile[curbatch] = NULL;
+		pfree(buf.data);
 	}
 
 	/*
@@ -1297,7 +1314,6 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 	{
 		uint32		hashvalue;
 		MinimalTuple tuple;
-		TupleTableSlot *slot;
 
 		if (!hashtable->batches[batchno].done)
 		{
@@ -1329,12 +1345,9 @@ ExecParallelHashJoinNewBatch(HashJoinState *hjstate)
 					while ((tuple = sts_parallel_scan_next(inner_tuples,
 														   &hashvalue)))
 					{
-						ExecForceStoreMinimalTuple(tuple,
-												   hjstate->hj_HashTupleSlot,
-												   false);
-						slot = hjstate->hj_HashTupleSlot;
-						ExecParallelHashTableInsertCurrentBatch(hashtable, slot,
-																hashvalue);
+						ExecParallelHashTableInsertCurrentBatchTuple(hashtable,
+																	 tuple,
+																	 hashvalue);
 					}
 					sts_end_parallel_scan(inner_tuples);
 					BarrierArriveAndWait(batch_barrier,
@@ -1448,14 +1461,14 @@ ExecHashJoinSaveTuple(MinimalTuple tuple, uint32 hashvalue,
  * ExecHashJoinGetSavedTuple
  *		read the next tuple from a batch file.  Return NULL if no more.
  *
- * On success, *hashvalue is set to the tuple's hash value, and the tuple
- * itself is stored in the given slot.
+ * On success, *hashvalue is set to the tuple's hash value, and return
+ * the tuple(stored in the given buf) itself.
  */
-static TupleTableSlot *
+static MinimalTuple
 ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 						  BufFile *file,
 						  uint32 *hashvalue,
-						  TupleTableSlot *tupleSlot)
+						  StringInfo buf)
 {
 	uint32		header[2];
 	size_t		nread;
@@ -1474,19 +1487,19 @@ ExecHashJoinGetSavedTuple(HashJoinState *hjstate,
 	 * cheating.
 	 */
 	nread = BufFileReadMaybeEOF(file, header, sizeof(header), true);
-	if (nread == 0)				/* end of file */
-	{
-		ExecClearTuple(tupleSlot);
+	if (unlikely(nread == 0))				/* end of file */
 		return NULL;
-	}
+
+	enlargeStringInfo(buf, header[1]);
 	*hashvalue = header[0];
-	tuple = (MinimalTuple) palloc(header[1]);
+	buf->len = header[1];
+	tuple = (MinimalTuple) buf->data;
 	tuple->t_len = header[1];
 	BufFileReadExact(file,
 					 (char *) tuple + sizeof(uint32),
 					 header[1] - sizeof(uint32));
-	ExecForceStoreMinimalTuple(tuple, tupleSlot, true);
-	return tupleSlot;
+
+	return tuple;
 }
 
 
diff --git a/src/include/executor/nodeHash.h b/src/include/executor/nodeHash.h
index e4eb7bc6359..37585035ade 100644
--- a/src/include/executor/nodeHash.h
+++ b/src/include/executor/nodeHash.h
@@ -36,12 +36,18 @@ extern void ExecParallelHashTableSetCurrentBatch(HashJoinTable hashtable,
 extern void ExecHashTableInsert(HashJoinTable hashtable,
 								TupleTableSlot *slot,
 								uint32 hashvalue);
+extern void ExecHashTableInsertTuple(HashJoinTable hashtable,
+									 MinimalTuple tuple,
+									 uint32 hashvalue);
 extern void ExecParallelHashTableInsert(HashJoinTable hashtable,
 										TupleTableSlot *slot,
 										uint32 hashvalue);
 extern void ExecParallelHashTableInsertCurrentBatch(HashJoinTable hashtable,
 													TupleTableSlot *slot,
 													uint32 hashvalue);
+extern void ExecParallelHashTableInsertCurrentBatchTuple(HashJoinTable hashtable,
+														 MinimalTuple tuple,
+														 uint32 hashvalue);
 extern void ExecHashGetBucketAndBatch(HashJoinTable hashtable,
 									  uint32 hashvalue,
 									  int *bucketno,
diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h
index af7d8fd1e72..299f66af135 100644
--- a/src/include/nodes/execnodes.h
+++ b/src/include/nodes/execnodes.h
@@ -2225,6 +2225,7 @@ typedef struct HashJoinState
 	TupleTableSlot *hj_NullOuterTupleSlot;
 	TupleTableSlot *hj_NullInnerTupleSlot;
 	TupleTableSlot *hj_FirstOuterTupleSlot;
+	StringInfo	hj_outerTupleBuffer;
 	int			hj_JoinState;
 	bool		hj_MatchedOuter;
 	bool		hj_OuterNotEmpty;
diff --git a/src/test/modules/unsafe_tests/Makefile b/src/test/modules/unsafe_tests/Makefile
index 90d19791871..a868a6ff34f 100644
--- a/src/test/modules/unsafe_tests/Makefile
+++ b/src/test/modules/unsafe_tests/Makefile
@@ -1,6 +1,6 @@
 # src/test/modules/unsafe_tests/Makefile
 
-REGRESS = rolenames alter_system_table guc_privs
+REGRESS = alter_system_table guc_privs
 
 # the whole point of these tests is to not run installcheck
 NO_INSTALLCHECK = 1


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

* Re: optimize hashjoin
@ 2024-08-21 17:17  Robert Haas <[email protected]>
  parent: Tomas Vondra <[email protected]>
  0 siblings, 0 replies; 4+ messages in thread

From: Robert Haas @ 2024-08-21 17:17 UTC (permalink / raw)
  To: Tomas Vondra <[email protected]>; +Cc: bucoo <[email protected]>; pgsql-hackers; Tom Lane <[email protected]>

On Wed, Aug 21, 2024 at 12:31 PM Tomas Vondra <[email protected]> wrote:
> Anyway, I took a look and the basic idea is simple - when shuffling
> tuples between batches in a hash join, we're currently deforming the
> tuple (->slot) we just read from a batch, only to immediately form it
> (slot->) again and write it to the "correct" batch.

Does skipping this cause any problem if some attributes are toasted?

I suppose not, just something to think about.

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






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


end of thread, other threads:[~2024-08-21 17:17 UTC | newest]

Thread overview: 4+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2021-02-27 04:03 [PATCH 01/10] Allow alternate compression methods for wal_compression Andrey Borodin <[email protected]>
2024-08-21 01:49 optimize hashjoin bucoo <[email protected]>
2024-08-21 16:31 ` Re: optimize hashjoin Tomas Vondra <[email protected]>
2024-08-21 17:17   ` Re: optimize hashjoin Robert Haas <[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