public inbox for [email protected]
help / color / mirror / Atom feed[PATCH 01/10] Allow alternate compression methods for wal_compression
5+ 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; 5+ 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] 5+ messages in thread
* pgstat include expansion
@ 2026-02-13 22:14 Andres Freund <[email protected]>
0 siblings, 2 replies; 5+ messages in thread
From: Andres Freund @ 2026-02-13 22:14 UTC (permalink / raw)
To: pgsql-hackers; Amit Kapila <[email protected]>; Alvaro Herrera <[email protected]>; Michael Paquier <[email protected]>
Hi,
A few recent-ish changes have substantially expanded the set of headers
included in pgstat.h. As pgstat.h is pretty widely included, that strikes me
as bad.
commit f6a4c498dcf
Author: Amit Kapila <[email protected]>
Date: 2025-11-07 08:05:08 +0000
Add seq_sync_error_count to subscription statistics.
added an include of replication/worker_internal.h, which in turn includes a
lot of other headers. It's also seems somewhat obvious that a header named
_internal.h shouldn't be included in something as widely included as pgstat.h
commit 7e638d7f509
Author: Álvaro Herrera <[email protected]>
Date: 2025-09-25 14:45:08 +0200
Don't include execnodes.h in replication/conflict.h
added an include of access/transam.h, which I don't love being included,
although it's less bad than some of the other cases. It's not actually to
blame for that though, as it shrank what was included noticeably.
commit 6c2b5edecc0
Author: Amit Kapila <[email protected]>
Date: 2024-09-04 08:55:21 +0530
Collect statistics about conflicts in logical replication.
added an include of conflict.h, which in turn includes utils/timestamp.h,
which includes fmgr.h (and a lot more, before 7e638d7f509).
I think now that we rely on C11, we actually could also forward-declare enum
typedefs. That would allow us to avoid including
worker_internal.h. Unfortunately I think C++ might throw a wrench in the mix
for that - IIUC it only allows forward declaring enums when using 'enum
class', but I don't think we can rely on that. I think the best solution
would be to move the typedef to a more suitable header, but baring that, I
guess just passing an int would do.
By forward declaring FullTransactionId, we can avoid including
access/transam.h
conflict.h includes utils/timestamp.h, even though it only needs the
types. Using datatype/timestamp.h suffices (although it requires some fixups
elsewhere).
conflict.h includes nodes/pg_list.h, which does in turn include nodes/nodes.h,
which, while not the worst, seems unnecessary. Another forward declaration
solves that.
The attached patch is just a prototype.
I also don't like that pgstat.h includes backend_status.h, which includes
things like pqcomm.h and miscadmin.h. But that's - leaving some moving around
aside - of a lot longer standing. We probably should move BackendType out of
miscadmin.h one of these days. Not sure what to do about the pqcomm.h
include...
Greetings,
Andres Freund
Attachments:
[text/x-diff] v1-0001-WIP-Reduce-includes-in-pgstat.h.patch (6.7K, ../../[email protected]/2-v1-0001-WIP-Reduce-includes-in-pgstat.h.patch)
download | inline diff:
From 92d8e460c4f65f8fca735d90f02fb128f53abeef Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Fri, 13 Feb 2026 17:05:15 -0500
Subject: [PATCH v1] WIP: Reduce includes in pgstat.h
---
src/include/pgstat.h | 13 +++++++------
src/include/replication/conflict.h | 5 +++--
src/backend/commands/functioncmds.c | 1 +
src/backend/executor/execReplication.c | 1 +
src/backend/replication/logical/conflict.c | 1 +
src/backend/storage/ipc/procsignal.c | 1 +
src/backend/utils/activity/pgstat_subscription.c | 4 ++--
.../test_custom_stats/test_custom_fixed_stats.c | 2 +-
.../test_custom_stats/test_custom_var_stats.c | 1 +
9 files changed, 18 insertions(+), 11 deletions(-)
diff --git a/src/include/pgstat.h b/src/include/pgstat.h
index fff7ecc2533..a2021a0e66f 100644
--- a/src/include/pgstat.h
+++ b/src/include/pgstat.h
@@ -11,18 +11,20 @@
#ifndef PGSTAT_H
#define PGSTAT_H
-#include "access/transam.h" /* for FullTransactionId */
#include "datatype/timestamp.h"
#include "portability/instr_time.h"
#include "postmaster/pgarch.h" /* for MAX_XFN_CHARS */
-#include "replication/conflict.h"
-#include "replication/worker_internal.h"
+#include "replication/conflict.h" /* for CONFLICT_NUM_TYPES */
#include "utils/backend_progress.h" /* for backward compatibility */ /* IWYU pragma: export */
#include "utils/backend_status.h" /* for backward compatibility */ /* IWYU pragma: export */
#include "utils/pgstat_kind.h"
-#include "utils/relcache.h"
#include "utils/wait_event.h" /* for backward compatibility */ /* IWYU pragma: export */
+/* to avoid including access/transam.h, for FullTransactionId */
+typedef struct FullTransactionId FullTransactionId;
+
+/* to avoid including utils/relcache.h */
+typedef struct RelationData *Relation;
/* ----------
* Paths for the statistics files (relative to installation's $PGDATA).
@@ -775,8 +777,7 @@ extern PgStat_SLRUStats *pgstat_fetch_slru(void);
* Functions in pgstat_subscription.c
*/
-extern void pgstat_report_subscription_error(Oid subid,
- LogicalRepWorkerType wtype);
+extern void pgstat_report_subscription_error(Oid subid, int wtype);
extern void pgstat_report_subscription_conflict(Oid subid, ConflictType type);
extern void pgstat_create_subscription(Oid subid);
extern void pgstat_drop_subscription(Oid subid);
diff --git a/src/include/replication/conflict.h b/src/include/replication/conflict.h
index 1cade336c91..4e1a8a2303f 100644
--- a/src/include/replication/conflict.h
+++ b/src/include/replication/conflict.h
@@ -10,14 +10,15 @@
#define CONFLICT_H
#include "access/xlogdefs.h"
-#include "nodes/pg_list.h"
-#include "utils/timestamp.h"
+#include "datatype/timestamp.h"
/* Avoid including execnodes.h here */
typedef struct EState EState;
typedef struct ResultRelInfo ResultRelInfo;
typedef struct TupleTableSlot TupleTableSlot;
+/* To avoid including pg_list.h */
+typedef struct List List;
/*
* Conflict types that could occur while applying remote changes.
diff --git a/src/backend/commands/functioncmds.c b/src/backend/commands/functioncmds.c
index a516b037dea..242372b1e68 100644
--- a/src/backend/commands/functioncmds.c
+++ b/src/backend/commands/functioncmds.c
@@ -34,6 +34,7 @@
#include "access/htup_details.h"
#include "access/table.h"
+#include "access/xact.h"
#include "catalog/catalog.h"
#include "catalog/dependency.h"
#include "catalog/indexing.h"
diff --git a/src/backend/executor/execReplication.c b/src/backend/executor/execReplication.c
index 743b1ee2b28..c1007c1b41b 100644
--- a/src/backend/executor/execReplication.c
+++ b/src/backend/executor/execReplication.c
@@ -27,6 +27,7 @@
#include "commands/trigger.h"
#include "executor/executor.h"
#include "executor/nodeModifyTable.h"
+#include "nodes/pg_list.h"
#include "replication/conflict.h"
#include "replication/logicalrelation.h"
#include "storage/lmgr.h"
diff --git a/src/backend/replication/logical/conflict.c b/src/backend/replication/logical/conflict.c
index ca71a81c7bf..73cca04ffa5 100644
--- a/src/backend/replication/logical/conflict.c
+++ b/src/backend/replication/logical/conflict.c
@@ -17,6 +17,7 @@
#include "access/commit_ts.h"
#include "access/tableam.h"
#include "executor/executor.h"
+#include "nodes/pg_list.h"
#include "pgstat.h"
#include "replication/conflict.h"
#include "replication/worker_internal.h"
diff --git a/src/backend/storage/ipc/procsignal.c b/src/backend/storage/ipc/procsignal.c
index 5d33559926a..4e32a29766c 100644
--- a/src/backend/storage/ipc/procsignal.c
+++ b/src/backend/storage/ipc/procsignal.c
@@ -23,6 +23,7 @@
#include "pgstat.h"
#include "port/pg_bitutils.h"
#include "replication/logicalworker.h"
+#include "replication/logicalctl.h"
#include "replication/walsender.h"
#include "storage/condition_variable.h"
#include "storage/ipc.h"
diff --git a/src/backend/utils/activity/pgstat_subscription.c b/src/backend/utils/activity/pgstat_subscription.c
index 500b1899188..adeff040075 100644
--- a/src/backend/utils/activity/pgstat_subscription.c
+++ b/src/backend/utils/activity/pgstat_subscription.c
@@ -25,7 +25,7 @@
* Report a subscription error.
*/
void
-pgstat_report_subscription_error(Oid subid, LogicalRepWorkerType wtype)
+pgstat_report_subscription_error(Oid subid, int wtype)
{
PgStat_EntryRef *entry_ref;
PgStat_BackendSubEntry *pending;
@@ -34,7 +34,7 @@ pgstat_report_subscription_error(Oid subid, LogicalRepWorkerType wtype)
InvalidOid, subid, NULL);
pending = entry_ref->pending;
- switch (wtype)
+ switch ((LogicalRepWorkerType) wtype)
{
case WORKERTYPE_APPLY:
pending->apply_error_count++;
diff --git a/src/test/modules/test_custom_stats/test_custom_fixed_stats.c b/src/test/modules/test_custom_stats/test_custom_fixed_stats.c
index 908bd18a7c7..fb74f3dc5ad 100644
--- a/src/test/modules/test_custom_stats/test_custom_fixed_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_fixed_stats.c
@@ -16,8 +16,8 @@
#include "access/htup_details.h"
#include "funcapi.h"
#include "pgstat.h"
-#include "utils/builtins.h"
#include "utils/pgstat_internal.h"
+#include "utils/timestamp.h"
PG_MODULE_MAGIC_EXT(
.name = "test_custom_fixed_stats",
diff --git a/src/test/modules/test_custom_stats/test_custom_var_stats.c b/src/test/modules/test_custom_stats/test_custom_var_stats.c
index 64a8fe63cce..da28afbd929 100644
--- a/src/test/modules/test_custom_stats/test_custom_var_stats.c
+++ b/src/test/modules/test_custom_stats/test_custom_var_stats.c
@@ -12,6 +12,7 @@
*/
#include "postgres.h"
+#include "access/htup_details.h"
#include "common/hashfn.h"
#include "funcapi.h"
#include "storage/dsm_registry.h"
--
2.46.0.519.g2e7b89e038
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgstat include expansion
@ 2026-02-16 04:36 Michael Paquier <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 1 reply; 5+ messages in thread
From: Michael Paquier @ 2026-02-16 04:36 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; Alvaro Herrera <[email protected]>
On Fri, Feb 13, 2026 at 05:14:01PM -0500, Andres Freund wrote:
> I think now that we rely on C11, we actually could also forward-declare enum
> typedefs. That would allow us to avoid including
> worker_internal.h. Unfortunately I think C++ might throw a wrench in the mix
> for that - IIUC it only allows forward declaring enums when using 'enum
> class', but I don't think we can rely on that. I think the best solution
> would be to move the typedef to a more suitable header, but baring that, I
> guess just passing an int would do.
> -extern void pgstat_report_subscription_error(Oid subid,
> - LogicalRepWorkerType wtype);
> +extern void pgstat_report_subscription_error(Oid subid, int wtype);
FWIW, I like some type enforcements when it comes to such report
routines. That avoids some careless assignments. This is usually a
sign of header refactoring to me, where the "light" declarations ought
to be moved into an independent header that can be fed back to other
places, like this one. An enum declaration or a set of constants can
be usually worth a split if their knowledge gets a lot across the
tree. That's just to say that while I agree about reducing the header
footprint, I don't find the result presented here to be the best thing
we can do.
--
Michael
Attachments:
[application/pgp-signature] signature.asc (833B, ../../[email protected]/2-signature.asc)
download
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgstat include expansion
@ 2026-02-16 17:18 Andres Freund <[email protected]>
parent: Michael Paquier <[email protected]>
0 siblings, 0 replies; 5+ messages in thread
From: Andres Freund @ 2026-02-16 17:18 UTC (permalink / raw)
To: Michael Paquier <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; Alvaro Herrera <[email protected]>
Hi,
On 2026-02-16 13:36:02 +0900, Michael Paquier wrote:
> On Fri, Feb 13, 2026 at 05:14:01PM -0500, Andres Freund wrote:
> > I think now that we rely on C11, we actually could also forward-declare enum
> > typedefs. That would allow us to avoid including
> > worker_internal.h. Unfortunately I think C++ might throw a wrench in the mix
> > for that - IIUC it only allows forward declaring enums when using 'enum
> > class', but I don't think we can rely on that. I think the best solution
> > would be to move the typedef to a more suitable header, but baring that, I
> > guess just passing an int would do.
>
>
> > -extern void pgstat_report_subscription_error(Oid subid,
> > - LogicalRepWorkerType wtype);
> > +extern void pgstat_report_subscription_error(Oid subid, int wtype);
>
> FWIW, I like some type enforcements when it comes to such report
> routines. That avoids some careless assignments.
In theory I agree (I after all did suggest moving the typedef to a better
header). However, the type enforcement argument IMO is somewhat bogus, as C
doesn't really have strong enum types, therefore you can just pass pretty
random stuff. I do wish C enums could be stronger...
> This is usually a sign of header refactoring to me, where the "light"
> declarations ought to be moved into an independent header that can be fed
> back to other places, like this one. An enum declaration or a set of
> constants can be usually worth a split if their knowledge gets a lot across
> the tree. That's just to say that while I agree about reducing the header
> footprint, I don't find the result presented here to be the best thing we
> can do.
I literally wrote that it isn't the best solution in the quoted portion above
and suggested moving the typedef to a different header? I was hoping the
author of the code would do that...
Greetings,
Andres Freund
^ permalink raw reply [nested|flat] 5+ messages in thread
* Re: pgstat include expansion
@ 2026-02-26 12:55 Alvaro Herrera <[email protected]>
parent: Andres Freund <[email protected]>
1 sibling, 0 replies; 5+ messages in thread
From: Alvaro Herrera @ 2026-02-26 12:55 UTC (permalink / raw)
To: Andres Freund <[email protected]>; +Cc: pgsql-hackers; Amit Kapila <[email protected]>; Michael Paquier <[email protected]>
Hello,
I committed the removal of transam.h and relcache.h from pgstat.h now.
No other files needed adjustment for these changes.
Thanks
--
Álvaro Herrera 48°01'N 7°57'E — https://www.EnterpriseDB.com/
"La fuerza no está en los medios físicos
sino que reside en una voluntad indomable" (Gandhi)
^ permalink raw reply [nested|flat] 5+ messages in thread
end of thread, other threads:[~2026-02-26 12:55 UTC | newest]
Thread overview: 5+ 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]>
2026-02-13 22:14 pgstat include expansion Andres Freund <[email protected]>
2026-02-16 04:36 ` Re: pgstat include expansion Michael Paquier <[email protected]>
2026-02-16 17:18 ` Re: pgstat include expansion Andres Freund <[email protected]>
2026-02-26 12:55 ` Re: pgstat include expansion Alvaro Herrera <[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