agora inbox for [email protected]
help / color / mirror / Atom feed[PATCH v10 12/17] Hard-code TBMIterateResult offsets array size
32+ messages / 8 participants
[nested] [flat]
* [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v10-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v12 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..1dc4c99bf99 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--6jpz2j246qmht4bt
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v12-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0011-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0011-table_scan_bitmap_next_block-counts-lossy-and-exa.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--3o7pc6dfau5a5hry
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v10-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--owzzsiozz6hgpp7e
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v11-0013-Separate-TBM-Shared-Iterator-and-TBMIterateResul.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0011-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0011-table_scan_bitmap_next_block-counts-lossy-and-exa.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--w4wcjcocxsm37usi
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v6-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 29 +++++++----------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 24 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fcc..d2bf8f44d50 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1467,8 +1453,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
* Create the TBMSharedIterator struct, with enough trailing space to
* serve the needs of the TBMIterateResult sub-struct.
*/
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639bf..432fae52962 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--kqqpqghcwbcc3dt5
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v7-0011-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--xqq4defy3uncu6k6
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v8-0011-table_scan_bitmap_next_block-counts-lossy-and-exa.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size
@ 2024-02-16 01:13 Melanie Plageman <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Melanie Plageman @ 2024-02-16 01:13 UTC (permalink / raw)
TIDBitmap's TBMIterateResult had a flexible sized array of tuple offsets
but the API always allocated MaxHeapTuplesPerPage OffsetNumbers.
Creating a fixed-size aray of size MaxHeapTuplesPerPage is more clear
for the API user.
---
src/backend/nodes/tidbitmap.c | 33 +++++++--------------------------
src/include/nodes/tidbitmap.h | 12 ++++++++++--
2 files changed, 17 insertions(+), 28 deletions(-)
diff --git a/src/backend/nodes/tidbitmap.c b/src/backend/nodes/tidbitmap.c
index e8ab5d78fc..1dc4c99bf9 100644
--- a/src/backend/nodes/tidbitmap.c
+++ b/src/backend/nodes/tidbitmap.c
@@ -40,7 +40,6 @@
#include <limits.h>
-#include "access/htup_details.h"
#include "common/hashfn.h"
#include "common/int.h"
#include "nodes/bitmapset.h"
@@ -48,14 +47,6 @@
#include "storage/lwlock.h"
#include "utils/dsa.h"
-/*
- * The maximum number of tuples per page is not large (typically 256 with
- * 8K pages, or 1024 with 32K pages). So there's not much point in making
- * the per-page bitmaps variable size. We just legislate that the size
- * is this:
- */
-#define MAX_TUPLES_PER_PAGE MaxHeapTuplesPerPage
-
/*
* When we have to switch over to lossy storage, we use a data structure
* with one bit per page, where all pages having the same number DIV
@@ -67,7 +58,7 @@
* table, using identical data structures. (This is because the memory
* management for hashtables doesn't easily/efficiently allow space to be
* transferred easily from one hashtable to another.) Therefore it's best
- * if PAGES_PER_CHUNK is the same as MAX_TUPLES_PER_PAGE, or at least not
+ * if PAGES_PER_CHUNK is the same as MaxHeapTuplesPerPage, or at least not
* too different. But we also want PAGES_PER_CHUNK to be a power of 2 to
* avoid expensive integer remainder operations. So, define it like this:
*/
@@ -79,7 +70,7 @@
#define BITNUM(x) ((x) % BITS_PER_BITMAPWORD)
/* number of active words for an exact page: */
-#define WORDS_PER_PAGE ((MAX_TUPLES_PER_PAGE - 1) / BITS_PER_BITMAPWORD + 1)
+#define WORDS_PER_PAGE ((MaxHeapTuplesPerPage - 1) / BITS_PER_BITMAPWORD + 1)
/* number of active words for a lossy chunk: */
#define WORDS_PER_CHUNK ((PAGES_PER_CHUNK - 1) / BITS_PER_BITMAPWORD + 1)
@@ -181,7 +172,7 @@ struct TBMIterator
int spageptr; /* next spages index */
int schunkptr; /* next schunks index */
int schunkbit; /* next bit to check in current schunk */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/*
@@ -222,7 +213,7 @@ struct TBMSharedIterator
PTEntryArray *ptbase; /* pagetable element array */
PTIterationArray *ptpages; /* sorted exact page index list */
PTIterationArray *ptchunks; /* sorted lossy page index list */
- TBMIterateResult output; /* MUST BE LAST (because variable-size) */
+ TBMIterateResult output;
};
/* Local function prototypes */
@@ -390,7 +381,7 @@ tbm_add_tuples(TIDBitmap *tbm, const ItemPointer tids, int ntids,
bitnum;
/* safety check to ensure we don't overrun bit array bounds */
- if (off < 1 || off > MAX_TUPLES_PER_PAGE)
+ if (off < 1 || off > MaxHeapTuplesPerPage)
elog(ERROR, "tuple offset out of range: %u", off);
/*
@@ -692,12 +683,7 @@ tbm_begin_iterate(TIDBitmap *tbm)
Assert(tbm->iterating != TBM_ITERATING_SHARED);
- /*
- * Create the TBMIterator struct, with enough trailing space to serve the
- * needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMIterator *) palloc(sizeof(TBMIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = palloc(sizeof(TBMIterator));
iterator->tbm = tbm;
/*
@@ -1463,12 +1449,7 @@ tbm_attach_shared_iterate(dsa_area *dsa, dsa_pointer dp)
TBMSharedIterator *iterator;
TBMSharedIteratorState *istate;
- /*
- * Create the TBMSharedIterator struct, with enough trailing space to
- * serve the needs of the TBMIterateResult sub-struct.
- */
- iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator) +
- MAX_TUPLES_PER_PAGE * sizeof(OffsetNumber));
+ iterator = (TBMSharedIterator *) palloc0(sizeof(TBMSharedIterator));
istate = (TBMSharedIteratorState *) dsa_get_address(dsa, dp);
diff --git a/src/include/nodes/tidbitmap.h b/src/include/nodes/tidbitmap.h
index 1945f0639b..432fae5296 100644
--- a/src/include/nodes/tidbitmap.h
+++ b/src/include/nodes/tidbitmap.h
@@ -22,6 +22,7 @@
#ifndef TIDBITMAP_H
#define TIDBITMAP_H
+#include "access/htup_details.h"
#include "storage/itemptr.h"
#include "utils/dsa.h"
@@ -41,9 +42,16 @@ typedef struct TBMIterateResult
{
BlockNumber blockno; /* page number containing tuples */
int ntuples; /* -1 indicates lossy result */
- bool recheck; /* should the tuples be rechecked? */
/* Note: recheck is always true if ntuples < 0 */
- OffsetNumber offsets[FLEXIBLE_ARRAY_MEMBER];
+ bool recheck; /* should the tuples be rechecked? */
+
+ /*
+ * The maximum number of tuples per page is not large (typically 256 with
+ * 8K pages, or 1024 with 32K pages). So there's not much point in making
+ * the per-page bitmaps variable size. We just legislate that the size is
+ * this:
+ */
+ OffsetNumber offsets[MaxHeapTuplesPerPage];
} TBMIterateResult;
/* function prototypes in nodes/tidbitmap.c */
--
2.40.1
--7mdtsjmrzitrgzgx
Content-Type: text/x-diff; charset=us-ascii
Content-Disposition: attachment;
filename="v9-0012-Separate-TBM-Shared-Iterator-and-TBMIterateResult.patch"
^ permalink raw reply [nested|flat] 32+ messages in thread
* Include extension path on pg_available_extensions
@ 2025-09-16 00:18 Matheus Alcantara <[email protected]>
0 siblings, 2 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-09-16 00:18 UTC (permalink / raw)
To: pgsql-hackers
Hi all,
On [1] it was mentioned that it could be a good idea to include the
extension location when listening the available extensions on
pg_available_extensions to make it clear to the user the location of an
extension that Postgres is seeing based on the extension_control_path
GUC.
The attached patch implements this idea. Extensions installed on $system
path will not show the actual value of the $system macro and it will
show the macro itself, for example:
postgres=# show extension_control_path;
extension_control_path
---------------------------------------------------
/usr/local/my/extensions/share/postgresql:$system
(1 row)
postgres=# select * from pg_available_extensions;
name | default_version | installed_version | comment | location
---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension
amcheck | 1.5 | | functions for verifying relation integrity | $system
bloom | 1.0 | | bloom access method - signature file based index | $system
I'm not sure if this should be included on 18 release since this is not
a bug fix but an improvement on the extension system by itself.
Any opinions on this?
[1] https://www.postgresql.org/message-id/CAKFQuwbR1Fzr8yRuMW%3DN1UMA1cTpFcqZe9bW_-ZF8%3DBa2Ud2%3Dw%40ma...
--
Matheus Alcantara
From 7a1c93f344c61c21f56692fe8eff77e5092929c0 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v1] Add path of extension on pg_available_extensions
---
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 80 ++++++++++++++-----
src/include/catalog/pg_proc.dat | 10 +--
.../t/001_extension_control_path.pl | 13 ++-
src/test/regress/expected/rules.out | 10 ++-
5 files changed, 83 insertions(+), 34 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index c77fa0234bb..8e3bc61fe4a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -402,14 +402,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.comment, E.location
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.comment, E.location
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..3f8e74f1211 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,19 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro is the macro plaeholder that the extension_control_path support
+ * and which is replaced by a system value that is stored on loc. For custom
+ * paths that don't have a macro the macro field is NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} Location;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +153,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ Location *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -354,7 +368,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ Location *location = palloc0_object(Location);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +384,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ Location *location = palloc0_object(Location);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +401,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2240,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(Location, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2254,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2284,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2279,6 +2304,12 @@ pg_available_extensions(PG_FUNCTION_ARGS)
else
values[2] = CStringGetTextDatum(control->comment);
+ /* location */
+ if (location->macro == NULL)
+ values[3] = CStringGetTextDatum(location->loc);
+ else
+ values[3] = CStringGetTextDatum(location->macro);
+
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
}
@@ -2313,9 +2344,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(Location, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2358,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2387,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2410,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ Location *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2424,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2434,6 +2467,12 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
else
values[7] = CStringGetTextDatum(control->comment);
+ /* location */
+ if (location->macro == NULL)
+ values[8] = CStringGetTextDatum(location->loc);
+ else
+ values[8] = CStringGetTextDatum(location->macro);
+
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
/*
@@ -2475,6 +2514,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
}
/* comment stays the same */
+ /* location stays the same */
+
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
}
@@ -3903,7 +3944,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ Location *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 03e82d28c87..4c2d16a4f6c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10735,16 +10735,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,comment,location}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment,location}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..332c74d72bc 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -43,29 +47,30 @@ is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
$node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
+
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extension_versions");
# Ensure that extensions installed on $system is still visible when using with
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 35e8aad7701..c876d2af0b8 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
- e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ e.comment,
+ e.location
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment, location)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
- e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ e.comment,
+ e.location
+ FROM (pg_available_extensions() e(name, default_version, comment, location)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
--
2.39.5 (Apple Git-154)
Attachments:
[text/plain] v1-0001-Add-path-of-extension-on-pg_available_extensions.patch (13.5K, ../../[email protected]/2-v1-0001-Add-path-of-extension-on-pg_available_extensions.patch)
download | inline diff:
From 7a1c93f344c61c21f56692fe8eff77e5092929c0 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v1] Add path of extension on pg_available_extensions
---
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 80 ++++++++++++++-----
src/include/catalog/pg_proc.dat | 10 +--
.../t/001_extension_control_path.pl | 13 ++-
src/test/regress/expected/rules.out | 10 ++-
5 files changed, 83 insertions(+), 34 deletions(-)
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index c77fa0234bb..8e3bc61fe4a 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -402,14 +402,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.comment, E.location
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.comment, E.location
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..3f8e74f1211 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,19 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro is the macro plaeholder that the extension_control_path support
+ * and which is replaced by a system value that is stored on loc. For custom
+ * paths that don't have a macro the macro field is NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} Location;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +153,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ Location *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -354,7 +368,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ Location *location = palloc0_object(Location);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +384,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ Location *location = palloc0_object(Location);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +401,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2240,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(Location, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2254,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2284,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2279,6 +2304,12 @@ pg_available_extensions(PG_FUNCTION_ARGS)
else
values[2] = CStringGetTextDatum(control->comment);
+ /* location */
+ if (location->macro == NULL)
+ values[3] = CStringGetTextDatum(location->loc);
+ else
+ values[3] = CStringGetTextDatum(location->macro);
+
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
}
@@ -2313,9 +2344,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(Location, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2358,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2387,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2410,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ Location *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2424,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2434,6 +2467,12 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
else
values[7] = CStringGetTextDatum(control->comment);
+ /* location */
+ if (location->macro == NULL)
+ values[8] = CStringGetTextDatum(location->loc);
+ else
+ values[8] = CStringGetTextDatum(location->macro);
+
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
/*
@@ -2475,6 +2514,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
}
/* comment stays the same */
+ /* location stays the same */
+
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
}
@@ -3903,7 +3944,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ Location *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 03e82d28c87..4c2d16a4f6c 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10735,16 +10735,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,comment,location}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment,location}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..332c74d72bc 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -43,29 +47,30 @@ is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
$node->safe_psql('postgres', "CREATE EXTENSION $ext_name");
$node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
+
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path|$ext_dir_canonicalized/extension",
"extension is installed correctly on pg_available_extension_versions");
# Ensure that extensions installed on $system is still visible when using with
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 35e8aad7701..c876d2af0b8 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
- e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ e.comment,
+ e.location
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment, location)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
- e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ e.comment,
+ e.location
+ FROM (pg_available_extensions() e(name, default_version, comment, location)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
--
2.39.5 (Apple Git-154)
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-02 15:11 Michael Banck <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 0 replies; 32+ messages in thread
From: Michael Banck @ 2025-11-02 15:11 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: pgsql-hackers
Hi,
On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote:
> postgres=# select * from pg_available_extensions;
> name | default_version | installed_version | comment | location
> ---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
> envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension
> amcheck | 1.5 | | functions for verifying relation integrity | $system
> bloom | 1.0 | | bloom access method - signature file based index | $system
I am not sure just adding the column at the end is best, I would have
put it before comment so that stays last, maybe somebody else has some
bikeshedding input here?
Michae
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-06 15:29 Matheus Alcantara <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-06 15:29 UTC (permalink / raw)
To: Michael Banck <[email protected]>; Matheus Alcantara <[email protected]>; +Cc: pgsql-hackers
Thanks for reviewing this!
On Sun Nov 2, 2025 at 12:11 PM -03, Michael Banck wrote:
> On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote:
>> postgres=# select * from pg_available_extensions;
>> name | default_version | installed_version | comment | location
>> ---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
>> envvar | 1.0.0 | | Get the value of a server environment variable | /usr/local/my/extensions/share/postgresql/extension
>> amcheck | 1.5 | | functions for verifying relation integrity | $system
>> bloom | 1.0 | | bloom access method - signature file based index | $system
>
> I am not sure just adding the column at the end is best, I would have
> put it before comment so that stays last, maybe somebody else has some
> bikeshedding input here?
>
Yeah, I think that it looks better to keep the comment at the end. If no
objections I'll swap the order of "comment" and "location" columns on
the next version.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-10 18:25 Manni Wood <[email protected]>
parent: Matheus Alcantara <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Manni Wood @ 2025-11-10 18:25 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Michael Banck <[email protected]>; pgsql-hackers
On Thu, Nov 6, 2025 at 9:29 AM Matheus Alcantara <[email protected]>
wrote:
> Thanks for reviewing this!
>
> On Sun Nov 2, 2025 at 12:11 PM -03, Michael Banck wrote:
> > On Mon, Sep 15, 2025 at 09:18:25PM -0300, Matheus Alcantara wrote:
> >> postgres=# select * from pg_available_extensions;
> >> name | default_version | installed_version |
> comment | location
> >>
> ---------+-----------------+-------------------+--------------------------------------------------+---------------------------------------------------
> >> envvar | 1.0.0 | | Get the value of a
> server environment variable |
> /usr/local/my/extensions/share/postgresql/extension
> >> amcheck | 1.5 | | functions for
> verifying relation integrity | $system
> >> bloom | 1.0 | | bloom access method -
> signature file based index | $system
> >
> > I am not sure just adding the column at the end is best, I would have
> > put it before comment so that stays last, maybe somebody else has some
> > bikeshedding input here?
> >
> Yeah, I think that it looks better to keep the comment at the end. If no
> objections I'll swap the order of "comment" and "location" columns on
> the next version.
>
> --
> Matheus Alcantara
> EDB: http://www.enterprisedb.com
>
>
>
>
Hello!
I have a small bikeshedding comment around making "location" the 4th column
returned for "select * from pg_available_extensions", as opposed to leaving
"comment" the 4th column returned for "select * from
pg_available_extensions".
If a bit of software runs "select * from pg_available_extensions" and
fetches the contents of the 4th column, that column will return "comment"
for current versions of postgres but "location" for patched versions of
postgres.
In many ways, this could be considered a feature and not a bug, because we
should be encouraged to write our SQL like so:
select name, default_version, installed_version, comment from
pg_available_extensions
and not like so:
select * from pg_available_extensions
I'm curious to know if this is a legitimate consideration or not.
Also, there were no surprises when I compiled and tested this: the location
shows correctly for a superuser, and "<insufficient privilege>" shows
correctly for a non-superuser.
--
-- Manni Wood EDB: https://www.enterprisedb.com
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-10 22:48 Matheus Alcantara <[email protected]>
parent: Manni Wood <[email protected]>
0 siblings, 2 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-10 22:48 UTC (permalink / raw)
To: Manni Wood <[email protected]>; Matheus Alcantara <[email protected]>; +Cc: Michael Banck <[email protected]>; pgsql-hackers
Thank you for reviewing this!
On Mon Nov 10, 2025 at 3:25 PM -03, Manni Wood wrote:
> Hello!
>
> I have a small bikeshedding comment around making "location" the 4th column
> returned for "select * from pg_available_extensions", as opposed to leaving
> "comment" the 4th column returned for "select * from
> pg_available_extensions".
>
> If a bit of software runs "select * from pg_available_extensions" and
> fetches the contents of the 4th column, that column will return "comment"
> for current versions of postgres but "location" for patched versions of
> postgres.
>
> In many ways, this could be considered a feature and not a bug, because we
> should be encouraged to write our SQL like so:
>
> select name, default_version, installed_version, comment from
> pg_available_extensions
>
> and not like so:
>
> select * from pg_available_extensions
>
> I'm curious to know if this is a legitimate consideration or not.
>
> Also, there were no surprises when I compiled and tested this: the location
> shows correctly for a superuser, and "<insufficient privilege>" shows
> correctly for a non-superuser.
>
Good point, I think that it's a legitimate consideration. That being
said I would get back to prefer to keep the location as the latest
column to avoid such issues even if SELECT * is not something that users
should do in practice, but I think that it's worth to avoid break any
application with such change.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-10 23:10 Michael Banck <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Michael Banck @ 2025-11-10 23:10 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Manni Wood <[email protected]>; pgsql-hackers
Hi,
On Mon, Nov 10, 2025 at 07:48:03PM -0300, Matheus Alcantara wrote:
> On Mon Nov 10, 2025 at 3:25 PM -03, Manni Wood wrote:
> > I have a small bikeshedding comment around making "location" the 4th column
> > returned for "select * from pg_available_extensions", as opposed to leaving
> > "comment" the 4th column returned for "select * from
> > pg_available_extensions".
> >
> > If a bit of software runs "select * from pg_available_extensions" and
> > fetches the contents of the 4th column, that column will return "comment"
> > for current versions of postgres but "location" for patched versions of
> > postgres.
> >
> > In many ways, this could be considered a feature and not a bug, because we
> > should be encouraged to write our SQL like so:
> >
> > select name, default_version, installed_version, comment from
> > pg_available_extensions
> >
> > and not like so:
> >
> > select * from pg_available_extensions
> >
> > I'm curious to know if this is a legitimate consideration or not.
> >
> > Also, there were no surprises when I compiled and tested this: the location
> > shows correctly for a superuser, and "<insufficient privilege>" shows
> > correctly for a non-superuser.
> >
> Good point, I think that it's a legitimate consideration. That being
> said I would get back to prefer to keep the location as the latest
> column to avoid such issues even if SELECT * is not something that users
> should do in practice, but I think that it's worth to avoid break any
> application with such change.
When the trusted column got added to the pg_availe_extensions view in
50fc694, it wasn't added to the end, but next to superuser, where it
logically makes sense IMO:
|@@ -317,7 +317,8 @@ CREATE VIEW pg_available_extensions AS
|
| CREATE VIEW pg_available_extension_versions AS
| SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
|- E.superuser, E.relocatable, E.schema, E.requires, E.comment
|+ E.superuser, E.trusted, E.relocatable,
|+ E.schema, E.requires, E.comment
| FROM pg_available_extension_versions() AS E
| LEFT JOIN pg_extension AS X
| ON E.name = X.extname AND E.version = X.extversion;
As far as I know, Postgres does not guarantee stable system catalogs
between major versions, so I don't think users should or could rely on
stable column ordering, really.
Michael
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-11 02:06 Rohit Prasad <[email protected]>
parent: Michael Banck <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Rohit Prasad @ 2025-11-11 02:06 UTC (permalink / raw)
To: [email protected]; +Cc: Matheus Alcantara <[email protected]>
Hi Michael,
I am just getting started in the Postgres community (this is my first code review). So please excuse me if I have missed something (in terms of process etc).
I reviewed your proposed code changes in the attached patch file and they look good to me. I have some minor comments:
1. In src/test/modules/test_extensions/t/001_extension_control_path.pl, it would be nice if you could add a test that validates that the correct Extension location is displayed, if for example, the extension is being picked up from a customized location.
2. Nit-pick: In src/backend/commands/extension.c:get_available_versions_for_extension(), you could probably combine the following 2 lines into one to say "'comment' & 'location' stay the same.
/* comment stays the same */
/* location stays the same */
Hope this is helpful.
Thanks,
-Rohit
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-11 11:24 Matheus Alcantara <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 0 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-11 11:24 UTC (permalink / raw)
To: Michael Banck <[email protected]>; Matheus Alcantara <[email protected]>; +Cc: Manni Wood <[email protected]>; pgsql-hackers
On Mon Nov 10, 2025 at 8:10 PM -03, Michael Banck wrote:
>> > I have a small bikeshedding comment around making "location" the 4th column
>> > returned for "select * from pg_available_extensions", as opposed to leaving
>> > "comment" the 4th column returned for "select * from
>> > pg_available_extensions".
>> >
>> > If a bit of software runs "select * from pg_available_extensions" and
>> > fetches the contents of the 4th column, that column will return "comment"
>> > for current versions of postgres but "location" for patched versions of
>> > postgres.
>> >
>> > In many ways, this could be considered a feature and not a bug, because we
>> > should be encouraged to write our SQL like so:
>> >
>> > select name, default_version, installed_version, comment from
>> > pg_available_extensions
>> >
>> > and not like so:
>> >
>> > select * from pg_available_extensions
>> >
>> > I'm curious to know if this is a legitimate consideration or not.
>> >
>> Good point, I think that it's a legitimate consideration. That being
>> said I would get back to prefer to keep the location as the latest
>> column to avoid such issues even if SELECT * is not something that users
>> should do in practice, but I think that it's worth to avoid break any
>> application with such change.
>
> When the trusted column got added to the pg_availe_extensions view in
> 50fc694, it wasn't added to the end, but next to superuser, where it
> logically makes sense IMO:
>
> |@@ -317,7 +317,8 @@ CREATE VIEW pg_available_extensions AS
> |
> | CREATE VIEW pg_available_extension_versions AS
> | SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
> |- E.superuser, E.relocatable, E.schema, E.requires, E.comment
> |+ E.superuser, E.trusted, E.relocatable,
> |+ E.schema, E.requires, E.comment
> | FROM pg_available_extension_versions() AS E
> | LEFT JOIN pg_extension AS X
> | ON E.name = X.extname AND E.version = X.extversion;
>
> As far as I know, Postgres does not guarantee stable system catalogs
> between major versions, so I don't think users should or could rely on
> stable column ordering, really.
>
Thanks for pointing this, I didn't know about this detail. I'm not
against swapping the orders of "comment" and "location" columns, I also
think that it would look better, I'm just afraid of breaking any
compatibility with anything, but it seems that it's not the case.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-11 12:47 Matheus Alcantara <[email protected]>
parent: Rohit Prasad <[email protected]>
0 siblings, 2 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-11 12:47 UTC (permalink / raw)
To: Rohit Prasad <[email protected]>; [email protected]; +Cc: [email protected]
On Mon Nov 10, 2025 at 11:06 PM -03, Rohit Prasad wrote:
> Hi Michael,
>
I think you wanted to say Matheus :)
> I am just getting started in the Postgres community (this is my first
> code review). So please excuse me if I have missed something (in terms
> of process etc).
>
Thank you for reviewing this!
> I reviewed your proposed code changes in the attached patch file and
> they look good to me.
>
Thanks.
> I have some minor comments:
> 1. In src/test/modules/test_extensions/t/001_extension_control_path.pl,
> it would be nice if you could add a test that validates that the
> correct Extension location is displayed, if for example, the extension
> is being picked up from a customized location.
>
I don't know if I get your point here. On the v4 patch we have:
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|Test extension_control_path|$ext_dir_canonicalized/extension",
The $ext_dir_canonicalized in this case is a custom path configured on
extension_control_path GUC that the "test_custom_ext_paths" extension
was installed.
> 2. Nit-pick: In
> src/backend/commands/extension.c:get_available_versions_for_extension(),
> you could probably combine the following 2 lines into one to say
> "'comment' & 'location' stay the same.
> /* comment stays the same */
> /* location stays the same */
>
Fixed on attached v5
On this new v5 version I also swap the order of "comment" and "location"
columns as it was suggested by Michael.
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
From 0ccd9aa0d8a5e4ba9168b16f14e8c617f023b4c1 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v5] Add path of extension on pg_available_extensions
Add a new "location" column to pg_available_extensions and
pg_available_extension_versions views. It exposes the directory that the
extension is located.
The default system location is show as $system macro, the same value
that is used to configure the extension_control_path GUC.
User-defined locations are only visible for users that has the
pg_read_extension_paths role, otherwise <insufficient privilege> is
returned as a column value, the same behaviour that we already have on
pg_stat_activity.
---
doc/src/sgml/system-views.sgml | 22 ++++
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 105 ++++++++++++++----
src/include/catalog/pg_proc.dat | 10 +-
.../t/001_extension_control_path.pl | 35 +++++-
src/test/regress/expected/rules.out | 6 +-
src/tools/pgindent/typedefs.list | 2 +-
7 files changed, 145 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7971498fe75..69b30bb6453 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -599,6 +599,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
@@ -723,6 +734,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 059e8778ca7..52fa2ddcaa3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,14 +412,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.location, E.comment
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.location, E.comment
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..9868f3684c6 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,20 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro field stores the name of a macro (for example “$system”) that
+ * extension_control_path supports, which is replaced by a system value that is
+ * stored in loc. For custom paths that don't have a macro the macro field is
+ * NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} ExtensionLocation;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +154,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ ExtensionLocation *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -157,6 +172,26 @@ static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
char *find_in_paths(const char *basename, List *paths);
+/*
+ * Return the extension location. If the current user doesn't have sufficient
+ * privileges, don't show the location.
+ */
+static char *
+get_extension_location(ExtensionLocation *loc)
+{
+ /* We only want to show extension paths for superusers. */
+ if (superuser_arg(GetUserId()))
+ {
+ /* Return the macro value if it's present to don't show system paths. */
+ if (loc->macro == NULL)
+ return loc->loc;
+ else
+ return loc->macro;
+ }
+ else
+ return "<insufficient privilege>";
+}
+
/*
* get_extension_oid - given an extension name, look up the OID
*
@@ -354,7 +389,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +405,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +422,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2261,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2275,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2305,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2273,11 +2319,15 @@ pg_available_extensions(PG_FUNCTION_ARGS)
nulls[1] = true;
else
values[1] = CStringGetTextDatum(control->default_version);
+
+ /* location */
+ values[2] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[2] = true;
+ nulls[3] = true;
else
- values[2] = CStringGetTextDatum(control->comment);
+ values[3] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
@@ -2313,9 +2363,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2377,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2406,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2429,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ ExtensionLocation *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2443,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2428,11 +2480,15 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
nulls[6] = true;
else
values[6] = convert_requires_to_datum(control->requires);
+
+ /* location */
+ values[7] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[7] = true;
+ nulls[8] = true;
else
- values[7] = CStringGetTextDatum(control->comment);
+ values[8] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
@@ -2473,7 +2529,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
values[6] = convert_requires_to_datum(control->requires);
nulls[6] = false;
}
- /* comment stays the same */
+ /* comment and location stays the same */
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
@@ -3903,7 +3959,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ ExtensionLocation *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5cf9e12fcb9..e34da1357e1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10743,16 +10743,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,location,comment}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,location,comment}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..11bfb77149d 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -35,6 +39,10 @@ extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/g
# Start node
$node->start;
+# Create an user to test permissions to read extension locations.
+my $user = "user01";
+$node->safe_psql('postgres', "CREATE USER $user");
+
my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
@@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
+# Test that a non-superuser can not read the extension location on
+# pg_available_extensions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extensions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extensions for insufficient privilege");
+
+# Test that a non-superuser can not read the extension location on
+# pg_available_extension_versions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extension_versions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extension_versions for insufficient privilege");
+
# Ensure that extensions installed on $system is still visible when using with
# custom extension control path.
$ret = $node->safe_psql('postgres',
@@ -76,7 +102,6 @@ $ret = $node->safe_psql('postgres',
is($ret, "t",
"\$system extension is installed correctly on pg_available_extensions");
-
$ret = $node->safe_psql('postgres',
"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7c52181cbcb..b402dc987ac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
+ e.location,
e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, location, comment)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
+ e.location,
e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ FROM (pg_available_extensions() e(name, default_version, location, comment)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 432509277c9..71dbf756a55 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -790,6 +790,7 @@ ExtensibleNodeEntry
ExtensibleNodeMethods
ExtensionControlFile
ExtensionInfo
+ExtensionLocation
ExtensionVersionInfo
FDWCollateState
FD_SET
@@ -1570,7 +1571,6 @@ LoadStmt
LocalBufferLookupEnt
LocalPgBackendStatus
LocalTransactionId
-Location
LocationIndex
LocationLen
LockAcquireResult
--
2.51.2
Attachments:
[text/plain] v5-0001-Add-path-of-extension-on-pg_available_extensions.patch (18.6K, ../../[email protected]/2-v5-0001-Add-path-of-extension-on-pg_available_extensions.patch)
download | inline diff:
From 0ccd9aa0d8a5e4ba9168b16f14e8c617f023b4c1 Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v5] Add path of extension on pg_available_extensions
Add a new "location" column to pg_available_extensions and
pg_available_extension_versions views. It exposes the directory that the
extension is located.
The default system location is show as $system macro, the same value
that is used to configure the extension_control_path GUC.
User-defined locations are only visible for users that has the
pg_read_extension_paths role, otherwise <insufficient privilege> is
returned as a column value, the same behaviour that we already have on
pg_stat_activity.
---
doc/src/sgml/system-views.sgml | 22 ++++
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 105 ++++++++++++++----
src/include/catalog/pg_proc.dat | 10 +-
.../t/001_extension_control_path.pl | 35 +++++-
src/test/regress/expected/rules.out | 6 +-
src/tools/pgindent/typedefs.list | 2 +-
7 files changed, 145 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7971498fe75..69b30bb6453 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -599,6 +599,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
@@ -723,6 +734,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 059e8778ca7..52fa2ddcaa3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,14 +412,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.location, E.comment
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.location, E.comment
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..9868f3684c6 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,20 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro field stores the name of a macro (for example “$system”) that
+ * extension_control_path supports, which is replaced by a system value that is
+ * stored in loc. For custom paths that don't have a macro the macro field is
+ * NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} ExtensionLocation;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +154,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ ExtensionLocation *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -157,6 +172,26 @@ static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
char *find_in_paths(const char *basename, List *paths);
+/*
+ * Return the extension location. If the current user doesn't have sufficient
+ * privileges, don't show the location.
+ */
+static char *
+get_extension_location(ExtensionLocation *loc)
+{
+ /* We only want to show extension paths for superusers. */
+ if (superuser_arg(GetUserId()))
+ {
+ /* Return the macro value if it's present to don't show system paths. */
+ if (loc->macro == NULL)
+ return loc->loc;
+ else
+ return loc->macro;
+ }
+ else
+ return "<insufficient privilege>";
+}
+
/*
* get_extension_oid - given an extension name, look up the OID
*
@@ -354,7 +389,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +405,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +422,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2261,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2275,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2305,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2273,11 +2319,15 @@ pg_available_extensions(PG_FUNCTION_ARGS)
nulls[1] = true;
else
values[1] = CStringGetTextDatum(control->default_version);
+
+ /* location */
+ values[2] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[2] = true;
+ nulls[3] = true;
else
- values[2] = CStringGetTextDatum(control->comment);
+ values[3] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
@@ -2313,9 +2363,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2377,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2406,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2429,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ ExtensionLocation *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2443,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2428,11 +2480,15 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
nulls[6] = true;
else
values[6] = convert_requires_to_datum(control->requires);
+
+ /* location */
+ values[7] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[7] = true;
+ nulls[8] = true;
else
- values[7] = CStringGetTextDatum(control->comment);
+ values[8] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
@@ -2473,7 +2529,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
values[6] = convert_requires_to_datum(control->requires);
nulls[6] = false;
}
- /* comment stays the same */
+ /* comment and location stays the same */
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
@@ -3903,7 +3959,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ ExtensionLocation *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5cf9e12fcb9..e34da1357e1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10743,16 +10743,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,location,comment}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,location,comment}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..11bfb77149d 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -35,6 +39,10 @@ extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/g
# Start node
$node->start;
+# Create an user to test permissions to read extension locations.
+my $user = "user01";
+$node->safe_psql('postgres', "CREATE USER $user");
+
my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
@@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
+# Test that a non-superuser can not read the extension location on
+# pg_available_extensions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extensions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extensions for insufficient privilege");
+
+# Test that a non-superuser can not read the extension location on
+# pg_available_extension_versions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extension_versions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extension_versions for insufficient privilege");
+
# Ensure that extensions installed on $system is still visible when using with
# custom extension control path.
$ret = $node->safe_psql('postgres',
@@ -76,7 +102,6 @@ $ret = $node->safe_psql('postgres',
is($ret, "t",
"\$system extension is installed correctly on pg_available_extensions");
-
$ret = $node->safe_psql('postgres',
"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7c52181cbcb..b402dc987ac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
+ e.location,
e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, location, comment)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
+ e.location,
e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ FROM (pg_available_extensions() e(name, default_version, location, comment)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 432509277c9..71dbf756a55 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -790,6 +790,7 @@ ExtensibleNodeEntry
ExtensibleNodeMethods
ExtensionControlFile
ExtensionInfo
+ExtensionLocation
ExtensionVersionInfo
FDWCollateState
FD_SET
@@ -1570,7 +1571,6 @@ LoadStmt
LocalBufferLookupEnt
LocalPgBackendStatus
LocalTransactionId
-Location
LocationIndex
LocationLen
LockAcquireResult
--
2.51.2
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-11 23:17 Rohit Prasad <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Rohit Prasad @ 2025-11-11 23:17 UTC (permalink / raw)
To: [email protected]; +Cc: Matheus Alcantara <[email protected]>
Hi Matheus,
Apologies for the mix up on the names :-)
I agree with your responses (please ignore my prior comment about test_extension code; I had missed some of your changes there).
I also reviewed v5 of your patch and the changes look good to me.
Thanks,
-Rohit
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-12 13:26 Matheus Alcantara <[email protected]>
parent: Rohit Prasad <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-12 13:26 UTC (permalink / raw)
To: Rohit Prasad <[email protected]>; [email protected]; +Cc: Matheus Alcantara <[email protected]>
On Tue Nov 11, 2025 at 8:17 PM -03, Rohit Prasad wrote:
> Hi Matheus,
>
> Apologies for the mix up on the names :-)
>
> I agree with your responses (please ignore my prior comment about test_extension code; I had missed some of your changes there).
>
> I also reviewed v5 of your patch and the changes look good to me.
>
No problem and thank you for reviewing!
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-13 09:11 Chao Li <[email protected]>
parent: Matheus Alcantara <[email protected]>
1 sibling, 1 reply; 32+ messages in thread
From: Chao Li @ 2025-11-13 09:11 UTC (permalink / raw)
To: Matheus Alcantara <[email protected]>; +Cc: Rohit Prasad <[email protected]>; [email protected]; [email protected]
Hi Matheus,
I just reviewed and tested the patch again, got a few more comments:
> On Nov 11, 2025, at 20:47, Matheus Alcantara <[email protected]> wrote:
>
>
> On this new v5 version I also swap the order of "comment" and "location"
> columns as it was suggested by Michael.
>
> --
> Matheus Alcantara
> EDB: http://www.enterprisedb.com
>
> <v5-0001-Add-path-of-extension-on-pg_available_extensions.patch>
1 - commit comment
```
User-defined locations are only visible for users that has the
pg_read_extension_paths role, otherwise <insufficient privilege> is
returned as a column value, the same behaviour that we already have on
pg_stat_activity.
```
First, there is a typo: "for users that has” should be “have”.
Then, Is this still true? The code change shows:
```
+ /* We only want to show extension paths for superusers. */
+ if (superuser_arg(GetUserId()))
+ {
```
And the code change says:
```
+ GUC. Only superusers can see the contents of this column.
```
But the TAP test contains a case for normal user:
```
+# Create an user to test permissions to read extension locations.
+my $user = "user01";
+$node->safe_psql('postgres', "CREATE USER $user");
+
my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
@@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions”);
```
And I tried to run the TAP test, it passed.
So I’m really confused here.
2
```
+ else
+ return "<insufficient privilege>";
+}
```
Why do we just show nothing (empty string)? Every row showing a long string of <insufficient privilege> just looks not good, that’s a lot of duplications.
```
evantest=> select * from pg_available_extension_versions;
name | version | installed | superuser | trusted | relocatable | schema | requires | location | comment
--------------------+---------+-----------+-----------+---------+-------------+------------+---------------------+--------------------------+------------------------------------------------------------------------
refint | 1.0 | f | t | f | t | | | <insufficient privilege> | functions for implementing referential integrity (obsolete)
unaccent | 1.1 | f | t | t | t | | | <insufficient privilege> | text search dictionary that removes accents
```
Best regards,
--
Chao Li (Evan)
HighGo Software Co., Ltd.
https://www.highgo.com/
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Include extension path on pg_available_extensions
@ 2025-11-13 14:36 Matheus Alcantara <[email protected]>
parent: Chao Li <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Matheus Alcantara @ 2025-11-13 14:36 UTC (permalink / raw)
To: Chao Li <[email protected]>; Matheus Alcantara <[email protected]>; +Cc: Rohit Prasad <[email protected]>; [email protected]; [email protected]
On Thu Nov 13, 2025 at 6:11 AM -03, Chao Li wrote:
>> <v5-0001-Add-path-of-extension-on-pg_available_extensions.patch>
>
> 1 - commit comment
> ```
> User-defined locations are only visible for users that has the
> pg_read_extension_paths role, otherwise <insufficient privilege> is
> returned as a column value, the same behaviour that we already have on
> pg_stat_activity.
> ```
>
> First, there is a typo: "for users that has” should be “have”.
>
> Then, Is this still true? The code change shows:
> ```
> + /* We only want to show extension paths for superusers. */
> + if (superuser_arg(GetUserId()))
> + {
> ```
>
> And the code change says:
> ```
> + GUC. Only superusers can see the contents of this column.
> ```
>
> But the TAP test contains a case for normal user:
> ```
> +# Create an user to test permissions to read extension locations.
> +my $user = "user01";
> +$node->safe_psql('postgres', "CREATE USER $user");
> +
> my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
>
> is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
> @@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
> my $ret = $node->safe_psql('postgres',
> "select * from pg_available_extensions where name = '$ext_name'");
> is( $ret,
> - "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
> + "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
> "extension is installed correctly on pg_available_extensions”);
> ```
>
> And I tried to run the TAP test, it passed.
>
> So I’m really confused here.
>
Oops, I forgot to update the commit message. The extension location is
only visible for super users. Fixed on v6.
The created user on TAP test is used to check that non superusers can not
see the location column content. The other test case changes are using
the 'postgres' user, so it should see the extension location value.
> 2
> ```
> + else
> + return "<insufficient privilege>";
> +}
> ```
>
> Why do we just show nothing (empty string)? Every row showing a long string of <insufficient privilege> just looks not good, that’s a lot of duplications.
>
This is the same behavior of pg_stat_activity. Returning an empty string
could make the user think that something is not right with the
implementation. Returning <insufficient privilege> for every row is a
lot of duplication I agree but I think that it make clear for the user
that it don't have the necessary role to view the column value.
Thanks for reviewing!
--
Matheus Alcantara
EDB: http://www.enterprisedb.com
From de163ebf950d9fe0152cc082a4bf26acc261aa2e Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v6] Add path of extension on pg_available_extensions
Add a new "location" column to pg_available_extensions and
pg_available_extension_versions views. It exposes the directory that the
extension is located.
The default system location is show as $system macro, the same value
that is used to configure the extension_control_path GUC.
User-defined locations are only visible for super users, otherwise
<insufficient privilege> is returned as a column value, the same
behaviour that we already have on pg_stat_activity.
---
doc/src/sgml/system-views.sgml | 22 ++++
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 105 ++++++++++++++----
src/include/catalog/pg_proc.dat | 10 +-
.../t/001_extension_control_path.pl | 35 +++++-
src/test/regress/expected/rules.out | 6 +-
src/tools/pgindent/typedefs.list | 2 +-
7 files changed, 145 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7971498fe75..69b30bb6453 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -599,6 +599,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
@@ -723,6 +734,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 059e8778ca7..52fa2ddcaa3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,14 +412,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.location, E.comment
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.location, E.comment
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..9868f3684c6 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,20 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro field stores the name of a macro (for example “$system”) that
+ * extension_control_path supports, which is replaced by a system value that is
+ * stored in loc. For custom paths that don't have a macro the macro field is
+ * NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} ExtensionLocation;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +154,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ ExtensionLocation *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -157,6 +172,26 @@ static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
char *find_in_paths(const char *basename, List *paths);
+/*
+ * Return the extension location. If the current user doesn't have sufficient
+ * privileges, don't show the location.
+ */
+static char *
+get_extension_location(ExtensionLocation *loc)
+{
+ /* We only want to show extension paths for superusers. */
+ if (superuser_arg(GetUserId()))
+ {
+ /* Return the macro value if it's present to don't show system paths. */
+ if (loc->macro == NULL)
+ return loc->loc;
+ else
+ return loc->macro;
+ }
+ else
+ return "<insufficient privilege>";
+}
+
/*
* get_extension_oid - given an extension name, look up the OID
*
@@ -354,7 +389,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +405,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +422,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2261,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2275,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2305,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2273,11 +2319,15 @@ pg_available_extensions(PG_FUNCTION_ARGS)
nulls[1] = true;
else
values[1] = CStringGetTextDatum(control->default_version);
+
+ /* location */
+ values[2] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[2] = true;
+ nulls[3] = true;
else
- values[2] = CStringGetTextDatum(control->comment);
+ values[3] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
@@ -2313,9 +2363,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2377,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2406,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2429,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ ExtensionLocation *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2443,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2428,11 +2480,15 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
nulls[6] = true;
else
values[6] = convert_requires_to_datum(control->requires);
+
+ /* location */
+ values[7] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[7] = true;
+ nulls[8] = true;
else
- values[7] = CStringGetTextDatum(control->comment);
+ values[8] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
@@ -2473,7 +2529,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
values[6] = convert_requires_to_datum(control->requires);
nulls[6] = false;
}
- /* comment stays the same */
+ /* comment and location stays the same */
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
@@ -3903,7 +3959,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ ExtensionLocation *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5cf9e12fcb9..e34da1357e1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10743,16 +10743,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,location,comment}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,location,comment}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..11bfb77149d 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -35,6 +39,10 @@ extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/g
# Start node
$node->start;
+# Create an user to test permissions to read extension locations.
+my $user = "user01";
+$node->safe_psql('postgres', "CREATE USER $user");
+
my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
@@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
+# Test that a non-superuser can not read the extension location on
+# pg_available_extensions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extensions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extensions for insufficient privilege");
+
+# Test that a non-superuser can not read the extension location on
+# pg_available_extension_versions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extension_versions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extension_versions for insufficient privilege");
+
# Ensure that extensions installed on $system is still visible when using with
# custom extension control path.
$ret = $node->safe_psql('postgres',
@@ -76,7 +102,6 @@ $ret = $node->safe_psql('postgres',
is($ret, "t",
"\$system extension is installed correctly on pg_available_extensions");
-
$ret = $node->safe_psql('postgres',
"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7c52181cbcb..b402dc987ac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
+ e.location,
e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, location, comment)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
+ e.location,
e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ FROM (pg_available_extensions() e(name, default_version, location, comment)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 23bce72ae64..d742dcc8335 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -790,6 +790,7 @@ ExtensibleNodeEntry
ExtensibleNodeMethods
ExtensionControlFile
ExtensionInfo
+ExtensionLocation
ExtensionVersionInfo
FDWCollateState
FD_SET
@@ -1570,7 +1571,6 @@ LoadStmt
LocalBufferLookupEnt
LocalPgBackendStatus
LocalTransactionId
-Location
LocationIndex
LocationLen
LockAcquireResult
--
2.51.2
Attachments:
[text/plain] v6-0001-Add-path-of-extension-on-pg_available_extensions.patch (18.6K, ../../[email protected]/2-v6-0001-Add-path-of-extension-on-pg_available_extensions.patch)
download | inline diff:
From de163ebf950d9fe0152cc082a4bf26acc261aa2e Mon Sep 17 00:00:00 2001
From: Matheus Alcantara <[email protected]>
Date: Mon, 15 Sep 2025 15:46:24 -0300
Subject: [PATCH v6] Add path of extension on pg_available_extensions
Add a new "location" column to pg_available_extensions and
pg_available_extension_versions views. It exposes the directory that the
extension is located.
The default system location is show as $system macro, the same value
that is used to configure the extension_control_path GUC.
User-defined locations are only visible for super users, otherwise
<insufficient privilege> is returned as a column value, the same
behaviour that we already have on pg_stat_activity.
---
doc/src/sgml/system-views.sgml | 22 ++++
src/backend/catalog/system_views.sql | 4 +-
src/backend/commands/extension.c | 105 ++++++++++++++----
src/include/catalog/pg_proc.dat | 10 +-
.../t/001_extension_control_path.pl | 35 +++++-
src/test/regress/expected/rules.out | 6 +-
src/tools/pgindent/typedefs.list | 2 +-
7 files changed, 145 insertions(+), 39 deletions(-)
diff --git a/doc/src/sgml/system-views.sgml b/doc/src/sgml/system-views.sgml
index 7971498fe75..69b30bb6453 100644
--- a/doc/src/sgml/system-views.sgml
+++ b/doc/src/sgml/system-views.sgml
@@ -599,6 +599,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
@@ -723,6 +734,17 @@
</para></entry>
</row>
+ <row>
+ <entry role="catalog_table_entry"><para role="column_definition">
+ <structfield>location</structfield> <type>text</type>
+ </para>
+ <para>
+ The location where the extension was found based on the <link
+ linkend="guc-extension-control-path"><structname>extension_control_path</structname></link>
+ GUC. Only superusers can see the contents of this column.
+ </para></entry>
+ </row>
+
<row>
<entry role="catalog_table_entry"><para role="column_definition">
<structfield>comment</structfield> <type>text</type>
diff --git a/src/backend/catalog/system_views.sql b/src/backend/catalog/system_views.sql
index 059e8778ca7..52fa2ddcaa3 100644
--- a/src/backend/catalog/system_views.sql
+++ b/src/backend/catalog/system_views.sql
@@ -412,14 +412,14 @@ CREATE VIEW pg_cursors AS
CREATE VIEW pg_available_extensions AS
SELECT E.name, E.default_version, X.extversion AS installed_version,
- E.comment
+ E.location, E.comment
FROM pg_available_extensions() AS E
LEFT JOIN pg_extension AS X ON E.name = X.extname;
CREATE VIEW pg_available_extension_versions AS
SELECT E.name, E.version, (X.extname IS NOT NULL) AS installed,
E.superuser, E.trusted, E.relocatable,
- E.schema, E.requires, E.comment
+ E.schema, E.requires, E.location, E.comment
FROM pg_available_extension_versions() AS E
LEFT JOIN pg_extension AS X
ON E.name = X.extname AND E.version = X.extversion;
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index 93ef1ad106f..9868f3684c6 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -126,6 +126,20 @@ typedef struct
ParseLoc stmt_len; /* length in bytes; 0 means "rest of string" */
} script_error_callback_arg;
+/*
+ * A location configured on extension_control_path GUC.
+ *
+ * The macro field stores the name of a macro (for example “$system”) that
+ * extension_control_path supports, which is replaced by a system value that is
+ * stored in loc. For custom paths that don't have a macro the macro field is
+ * NULL.
+ */
+typedef struct
+{
+ char *macro;
+ char *loc;
+} ExtensionLocation;
+
/* Local functions */
static List *find_update_path(List *evi_list,
ExtensionVersionInfo *evi_start,
@@ -140,7 +154,8 @@ static Oid get_required_extension(char *reqExtensionName,
bool is_create);
static void get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc);
+ TupleDesc tupdesc,
+ ExtensionLocation *location);
static Datum convert_requires_to_datum(List *requires);
static void ApplyExtensionUpdates(Oid extensionOid,
ExtensionControlFile *pcontrol,
@@ -157,6 +172,26 @@ static ExtensionControlFile *new_ExtensionControlFile(const char *extname);
char *find_in_paths(const char *basename, List *paths);
+/*
+ * Return the extension location. If the current user doesn't have sufficient
+ * privileges, don't show the location.
+ */
+static char *
+get_extension_location(ExtensionLocation *loc)
+{
+ /* We only want to show extension paths for superusers. */
+ if (superuser_arg(GetUserId()))
+ {
+ /* Return the macro value if it's present to don't show system paths. */
+ if (loc->macro == NULL)
+ return loc->loc;
+ else
+ return loc->macro;
+ }
+ else
+ return "<insufficient privilege>";
+}
+
/*
* get_extension_oid - given an extension name, look up the OID
*
@@ -354,7 +389,11 @@ get_extension_control_directories(void)
if (strlen(Extension_control_path) == 0)
{
- paths = lappend(paths, system_dir);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
+
+ location->macro = NULL;
+ location->loc = system_dir;
+ paths = lappend(paths, location);
}
else
{
@@ -366,6 +405,7 @@ get_extension_control_directories(void)
int len;
char *mangled;
char *piece = first_path_var_separator(ecp);
+ ExtensionLocation *location = palloc_object(ExtensionLocation);
/* Get the length of the next path on ecp */
if (piece == NULL)
@@ -382,15 +422,21 @@ get_extension_control_directories(void)
* suffix if it is a custom extension control path.
*/
if (strcmp(piece, "$system") == 0)
+ {
+ location->macro = pstrdup(piece);
mangled = substitute_path_macro(piece, "$system", system_dir);
+ }
else
+ {
+ location->macro = NULL;
mangled = psprintf("%s/extension", piece);
-
+ }
pfree(piece);
/* Canonicalize the path based on the OS and add to the list */
canonicalize_path(mangled);
- paths = lappend(paths, mangled);
+ location->loc = mangled;
+ paths = lappend(paths, location);
/* Break if ecp is empty or move to the next path on ecp */
if (ecp[len] == '\0')
@@ -2215,9 +2261,9 @@ pg_available_extensions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2229,13 +2275,13 @@ pg_available_extensions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
String *extname_str;
- Datum values[3];
- bool nulls[3];
+ Datum values[4];
+ bool nulls[4];
if (!is_extension_control_filename(de->d_name))
continue;
@@ -2259,7 +2305,7 @@ pg_available_extensions(PG_FUNCTION_ARGS)
found_ext = lappend(found_ext, extname_str);
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
memset(values, 0, sizeof(values));
@@ -2273,11 +2319,15 @@ pg_available_extensions(PG_FUNCTION_ARGS)
nulls[1] = true;
else
values[1] = CStringGetTextDatum(control->default_version);
+
+ /* location */
+ values[2] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[2] = true;
+ nulls[3] = true;
else
- values[2] = CStringGetTextDatum(control->comment);
+ values[3] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(rsinfo->setResult, rsinfo->setDesc,
values, nulls);
@@ -2313,9 +2363,9 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
locations = get_extension_control_directories();
- foreach_ptr(char, location, locations)
+ foreach_ptr(ExtensionLocation, location, locations)
{
- dir = AllocateDir(location);
+ dir = AllocateDir(location->loc);
/*
* If the control directory doesn't exist, we want to silently return
@@ -2327,7 +2377,7 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
}
else
{
- while ((de = ReadDir(dir, location)) != NULL)
+ while ((de = ReadDir(dir, location->loc)) != NULL)
{
ExtensionControlFile *control;
char *extname;
@@ -2356,12 +2406,13 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
/* read the control file */
control = new_ExtensionControlFile(extname);
- control->control_dir = pstrdup(location);
+ control->control_dir = pstrdup(location->loc);
parse_extension_control_file(control, NULL);
/* scan extension's script directory for install scripts */
get_available_versions_for_extension(control, rsinfo->setResult,
- rsinfo->setDesc);
+ rsinfo->setDesc,
+ location);
}
FreeDir(dir);
@@ -2378,7 +2429,8 @@ pg_available_extension_versions(PG_FUNCTION_ARGS)
static void
get_available_versions_for_extension(ExtensionControlFile *pcontrol,
Tuplestorestate *tupstore,
- TupleDesc tupdesc)
+ TupleDesc tupdesc,
+ ExtensionLocation *location)
{
List *evi_list;
ListCell *lc;
@@ -2391,8 +2443,8 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
{
ExtensionVersionInfo *evi = (ExtensionVersionInfo *) lfirst(lc);
ExtensionControlFile *control;
- Datum values[8];
- bool nulls[8];
+ Datum values[9];
+ bool nulls[9];
ListCell *lc2;
if (!evi->installable)
@@ -2428,11 +2480,15 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
nulls[6] = true;
else
values[6] = convert_requires_to_datum(control->requires);
+
+ /* location */
+ values[7] = CStringGetTextDatum(get_extension_location(location));
+
/* comment */
if (control->comment == NULL)
- nulls[7] = true;
+ nulls[8] = true;
else
- values[7] = CStringGetTextDatum(control->comment);
+ values[8] = CStringGetTextDatum(control->comment);
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
@@ -2473,7 +2529,7 @@ get_available_versions_for_extension(ExtensionControlFile *pcontrol,
values[6] = convert_requires_to_datum(control->requires);
nulls[6] = false;
}
- /* comment stays the same */
+ /* comment and location stays the same */
tuplestore_putvalues(tupstore, tupdesc, values, nulls);
}
@@ -3903,7 +3959,8 @@ find_in_paths(const char *basename, List *paths)
foreach(cell, paths)
{
- char *path = lfirst(cell);
+ ExtensionLocation *location = lfirst(cell);
+ char *path = location->loc;
char *full;
Assert(path != NULL);
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 5cf9e12fcb9..e34da1357e1 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -10743,16 +10743,16 @@
{ oid => '3082', descr => 'list available extensions',
proname => 'pg_available_extensions', procost => '10', prorows => '100',
proretset => 't', provolatile => 's', prorettype => 'record',
- proargtypes => '', proallargtypes => '{name,text,text}',
- proargmodes => '{o,o,o}', proargnames => '{name,default_version,comment}',
+ proargtypes => '', proallargtypes => '{name,text,text,text}',
+ proargmodes => '{o,o,o,o}', proargnames => '{name,default_version,location,comment}',
prosrc => 'pg_available_extensions' },
{ oid => '3083', descr => 'list available extension versions',
proname => 'pg_available_extension_versions', procost => '10',
prorows => '100', proretset => 't', provolatile => 's',
prorettype => 'record', proargtypes => '',
- proallargtypes => '{name,text,bool,bool,bool,name,_name,text}',
- proargmodes => '{o,o,o,o,o,o,o,o}',
- proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,comment}',
+ proallargtypes => '{name,text,bool,bool,bool,name,_name,text,text}',
+ proargmodes => '{o,o,o,o,o,o,o,o,o}',
+ proargnames => '{name,version,superuser,trusted,relocatable,schema,requires,location,comment}',
prosrc => 'pg_available_extension_versions' },
{ oid => '3084', descr => 'list an extension\'s version update paths',
proname => 'pg_extension_update_paths', procost => '10', prorows => '100',
diff --git a/src/test/modules/test_extensions/t/001_extension_control_path.pl b/src/test/modules/test_extensions/t/001_extension_control_path.pl
index 7fbe5bde332..11bfb77149d 100644
--- a/src/test/modules/test_extensions/t/001_extension_control_path.pl
+++ b/src/test/modules/test_extensions/t/001_extension_control_path.pl
@@ -25,6 +25,10 @@ my $ext_name2 = "test_custom_ext_paths_using_directory";
mkpath("$ext_dir/$ext_name2");
create_extension($ext_name2, $ext_dir, $ext_name2);
+# Make windows path use Unix slashes as canonicalize_path() is called when
+# collecting extension control paths. See get_extension_control_directories().
+my $ext_dir_canonicalized = $windows_os ? ($ext_dir =~ s/\\/\//gr) : $ext_dir;
+
# Use the correct separator and escape \ when running on Windows.
my $sep = $windows_os ? ";" : ":";
$node->append_conf(
@@ -35,6 +39,10 @@ extension_control_path = '\$system$sep@{[ $windows_os ? ($ext_dir =~ s/\\/\\\\/g
# Start node
$node->start;
+# Create an user to test permissions to read extension locations.
+my $user = "user01";
+$node->safe_psql('postgres', "CREATE USER $user");
+
my $ecp = $node->safe_psql('postgres', 'show extension_control_path;');
is($ecp, "\$system$sep$ext_dir$sep$ext_dir2",
@@ -46,28 +54,46 @@ $node->safe_psql('postgres', "CREATE EXTENSION $ext_name2");
my $ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name'");
is( $ret,
- "test_custom_ext_paths|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extensions where name = '$ext_name2'");
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|1.0|Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|1.0|$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extensions");
$ret = $node->safe_psql('postgres',
"select * from pg_available_extension_versions where name = '$ext_name2'"
);
is( $ret,
- "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||Test extension_control_path",
+ "test_custom_ext_paths_using_directory|1.0|t|t|f|t|||$ext_dir_canonicalized/extension|Test extension_control_path",
"extension is installed correctly on pg_available_extension_versions");
+# Test that a non-superuser can not read the extension location on
+# pg_available_extensions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extensions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extensions for insufficient privilege");
+
+# Test that a non-superuser can not read the extension location on
+# pg_available_extension_versions
+$ret = $node->safe_psql('postgres',
+ "select location from pg_available_extension_versions where name = '$ext_name2'",
+ connstr => "user=$user");
+is( $ret,
+ "<insufficient privilege>",
+ "extension location is hide on pg_available_extension_versions for insufficient privilege");
+
# Ensure that extensions installed on $system is still visible when using with
# custom extension control path.
$ret = $node->safe_psql('postgres',
@@ -76,7 +102,6 @@ $ret = $node->safe_psql('postgres',
is($ret, "t",
"\$system extension is installed correctly on pg_available_extensions");
-
$ret = $node->safe_psql('postgres',
"set extension_control_path = ''; select count(*) > 0 as ok from pg_available_extensions where name = 'plpgsql'"
);
diff --git a/src/test/regress/expected/rules.out b/src/test/regress/expected/rules.out
index 7c52181cbcb..b402dc987ac 100644
--- a/src/test/regress/expected/rules.out
+++ b/src/test/regress/expected/rules.out
@@ -1310,14 +1310,16 @@ pg_available_extension_versions| SELECT e.name,
e.relocatable,
e.schema,
e.requires,
+ e.location,
e.comment
- FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, comment)
+ FROM (pg_available_extension_versions() e(name, version, superuser, trusted, relocatable, schema, requires, location, comment)
LEFT JOIN pg_extension x ON (((e.name = x.extname) AND (e.version = x.extversion))));
pg_available_extensions| SELECT e.name,
e.default_version,
x.extversion AS installed_version,
+ e.location,
e.comment
- FROM (pg_available_extensions() e(name, default_version, comment)
+ FROM (pg_available_extensions() e(name, default_version, location, comment)
LEFT JOIN pg_extension x ON ((e.name = x.extname)));
pg_backend_memory_contexts| SELECT name,
ident,
diff --git a/src/tools/pgindent/typedefs.list b/src/tools/pgindent/typedefs.list
index 23bce72ae64..d742dcc8335 100644
--- a/src/tools/pgindent/typedefs.list
+++ b/src/tools/pgindent/typedefs.list
@@ -790,6 +790,7 @@ ExtensibleNodeEntry
ExtensibleNodeMethods
ExtensionControlFile
ExtensionInfo
+ExtensionLocation
ExtensionVersionInfo
FDWCollateState
FD_SET
@@ -1570,7 +1571,6 @@ LoadStmt
LocalBufferLookupEnt
LocalPgBackendStatus
LocalTransactionId
-Location
LocationIndex
LocationLen
LockAcquireResult
--
2.51.2
^ permalink raw reply [nested|flat] 32+ messages in thread
* Generate images for docs by using meson build system
@ 2025-12-22 11:46 Nazir Bilal Yavuz <[email protected]>
0 siblings, 1 reply; 32+ messages in thread
From: Nazir Bilal Yavuz @ 2025-12-22 11:46 UTC (permalink / raw)
To: PostgreSQL Hackers <[email protected]>; +Cc: Daniel Gustafsson <[email protected]>
Hi,
Daniel pinged me off-list about that we do not have a way to generate
images for docs by using the meson build system, attached fixes that.
In addition to that, I think we might want to update README for ditaa
since its URL is pointing to the wrong version. I created another
thread for this [1].
[1] https://postgr.es/m/CAN55FZ2O-23xERF2NYcvv9DM_1c9T16y6mi3vyP%3DO1iuXS0ASA%40mail.gmail.com
--
Regards,
Nazir Bilal Yavuz
Microsoft
Attachments:
[text/x-patch] v1-0001-meson-Generate-images-for-docs-by-using-meson.patch (3.2K, ../../CAN55FZ0c0Tcjx9=e-YibWGHa1-xmdV63p=THH4YYznz+pYcfig@mail.gmail.com/2-v1-0001-meson-Generate-images-for-docs-by-using-meson.patch)
download | inline diff:
From 9999c1dca65526e17ad1ac2f99bb1e0a33b9c1ce Mon Sep 17 00:00:00 2001
From: Nazir Bilal Yavuz <[email protected]>
Date: Mon, 22 Dec 2025 14:10:54 +0300
Subject: [PATCH v1] meson: Generate images for docs by using meson
There was no way to generate images for docs by using meson build
system. So, add a way to do that by adding a `images` target to the
meson build. Images can be generated by using `meson compile images`
command now.
Reported-by: Daniel Gustafsson <[email protected]>
---
doc/src/sgml/images/meson.build | 61 +++++++++++++++++++++++++++++++++
doc/src/sgml/meson.build | 2 ++
meson.build | 2 ++
3 files changed, 65 insertions(+)
create mode 100644 doc/src/sgml/images/meson.build
diff --git a/doc/src/sgml/images/meson.build b/doc/src/sgml/images/meson.build
new file mode 100644
index 00000000000..8e601e877a5
--- /dev/null
+++ b/doc/src/sgml/images/meson.build
@@ -0,0 +1,61 @@
+# doc/src/sgml/images/meson.build
+#
+# see README in this directory about image handling
+
+if not xsltproc_bin.found() or not dot.found() or not ditaa.found()
+ subdir_done()
+endif
+
+image_targets = []
+
+fixup_svg_xsl = files('fixup-svg.xsl')
+
+all_files = [
+ 'genetic-algorithm.gv',
+ 'gin.gv',
+ 'pagelayout.txt',
+ 'temporal-entities.txt',
+ 'temporal-references.txt',
+]
+
+foreach file : all_files
+
+ str_split = file.split('.')
+ actual_file_name = str_split[0]
+ extension = str_split[1]
+ cur_file = files(file)
+ tmp_name = '@[email protected]'.format(file)
+ output_name = '@[email protected]'.format(actual_file_name)
+
+ command = []
+ if extension == 'gv'
+ command = [dot, '-T', 'svg', '-o', '@OUTPUT@', '@INPUT@']
+ elif extension == 'txt'
+ command = [ditaa, '-E', '-S', '--svg', '@INPUT@', '@OUTPUT@']
+ else
+ error('Unknown extension: ".@0@" while generating images'.format(extension))
+ endif
+
+ svg_tmp = custom_target(tmp_name,
+ input: cur_file,
+ output: tmp_name,
+ command: command,
+ )
+
+ current_svg = custom_target(output_name,
+ input: svg_tmp,
+ output: output_name,
+ command: [xsltproc_bin,
+ '--nonet',
+ # Use --novalid to avoid loading SVG DTD if a file specifies it, since
+ # it might not be available locally, and we don't need it.
+ '--novalid',
+ '-o', '@OUTPUT@',
+ fixup_svg_xsl,
+ '@INPUT@']
+ )
+
+ image_targets += current_svg
+endforeach
+
+alias_target('images', image_targets)
diff --git a/doc/src/sgml/meson.build b/doc/src/sgml/meson.build
index 6ae192eac68..6852ac7e57c 100644
--- a/doc/src/sgml/meson.build
+++ b/doc/src/sgml/meson.build
@@ -1,5 +1,7 @@
# Copyright (c) 2022-2025, PostgreSQL Global Development Group
+subdir('images')
+
docs = []
installdocs = []
alldocs = []
diff --git a/meson.build b/meson.build
index d7c5193d4ce..34b748c8f35 100644
--- a/meson.build
+++ b/meson.build
@@ -351,6 +351,8 @@ cp = find_program('cp', required: false, native: true)
xmllint_bin = find_program(get_option('XMLLINT'), native: true, required: false)
xsltproc_bin = find_program(get_option('XSLTPROC'), native: true, required: false)
nm = find_program('nm', required: false, native: true)
+ditaa = find_program('ditaa', native: true, required: false)
+dot = find_program('dot', native: true, required: false)
bison_flags = []
if bison.found()
--
2.47.3
^ permalink raw reply [nested|flat] 32+ messages in thread
* Re: Generate images for docs by using meson build system
@ 2026-02-13 10:52 Daniel Gustafsson <[email protected]>
parent: Nazir Bilal Yavuz <[email protected]>
0 siblings, 0 replies; 32+ messages in thread
From: Daniel Gustafsson @ 2026-02-13 10:52 UTC (permalink / raw)
To: Nazir Bilal Yavuz <[email protected]>; +Cc: PostgreSQL Hackers <[email protected]>
> On 22 Dec 2025, at 12:46, Nazir Bilal Yavuz <[email protected]> wrote:
> Daniel pinged me off-list about that we do not have a way to generate
> images for docs by using the meson build system, attached fixes that.
Thanks for the fix, and sorry for the late reply. I've pushed this now.
> In addition to that, I think we might want to update README for ditaa
> since its URL is pointing to the wrong version. I created another
> thread for this [1].
I pushed this as well with a little bit of wordsmithing applied.
--
Daniel Gustafsson
^ permalink raw reply [nested|flat] 32+ messages in thread
end of thread, other threads:[~2026-02-13 10:52 UTC | newest]
Thread overview: 32+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2024-02-16 01:13 [PATCH v12 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v11 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v10 12/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v8 10/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v7 10/13] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v6 11/14] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2024-02-16 01:13 [PATCH v9 11/17] Hard-code TBMIterateResult offsets array size Melanie Plageman <[email protected]>
2025-09-16 00:18 Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-02 15:11 ` Re: Include extension path on pg_available_extensions Michael Banck <[email protected]>
2025-11-06 15:29 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-10 18:25 ` Re: Include extension path on pg_available_extensions Manni Wood <[email protected]>
2025-11-10 22:48 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-10 23:10 ` Re: Include extension path on pg_available_extensions Michael Banck <[email protected]>
2025-11-11 02:06 ` Re: Include extension path on pg_available_extensions Rohit Prasad <[email protected]>
2025-11-11 12:47 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-11 23:17 ` Re: Include extension path on pg_available_extensions Rohit Prasad <[email protected]>
2025-11-12 13:26 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-13 09:11 ` Re: Include extension path on pg_available_extensions Chao Li <[email protected]>
2025-11-13 14:36 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-11-11 11:24 ` Re: Include extension path on pg_available_extensions Matheus Alcantara <[email protected]>
2025-12-22 11:46 Generate images for docs by using meson build system Nazir Bilal Yavuz <[email protected]>
2026-02-13 10:52 ` Re: Generate images for docs by using meson build system Daniel Gustafsson <[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