Restore support for USE_ASSERT_CHECKING in extensions only
Prior to 6f3820f, extensions could be compiled with -DUSE_ASSERT_CHECKING
whether or not the Postgres installation was configured with
--enable-cassert (to enable at least some assertion checking). However,
after 6f3820f, linking fails with `undefined symbol:
verify_compact_attribute`. I'm not sure if this use case is officially
supported, but it has been helpful for developing pgvector.
The attached patch fixes it, but there may be a better way to do this.
Attachments:
v1-0001-Restore-support-for-USE_ASSERT_CHECKING-in-extens.patchapplication/octet-stream; name=v1-0001-Restore-support-for-USE_ASSERT_CHECKING-in-extens.patchDownload
From 94a517328c103fdd8b4a7a0292d4150840fde4f8 Mon Sep 17 00:00:00 2001
From: Andrew Kane <andrew@ankane.org>
Date: Fri, 10 Jan 2025 14:11:52 -0800
Subject: [PATCH v1] Restore support for USE_ASSERT_CHECKING in extensions only
---
src/backend/access/common/tupdesc.c | 3 ---
src/include/access/tupdesc.h | 2 --
2 files changed, 5 deletions(-)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index cc94074219..7d69414b7f 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -118,7 +118,6 @@ populate_compact_attribute(TupleDesc tupdesc, int attnum)
populate_compact_attribute_internal(src, dst);
}
-#ifdef USE_ASSERT_CHECKING
/*
* verify_compact_attribute
* In Assert enabled builds, we verify that the CompactAttribute is
@@ -151,8 +150,6 @@ verify_compact_attribute(TupleDesc tupdesc, int attnum)
/* Check the freshly populated CompactAttribute matches the TupleDesc's */
Assert(memcmp(&tmp, cattr, sizeof(CompactAttribute)) == 0);
}
-#endif
-
/*
* CreateTemplateTupleDesc
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index aee871b0e8..504ce22250 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -158,9 +158,7 @@ TupleDescAttr(TupleDesc tupdesc, int i)
#undef TupleDescAttrAddress
-#ifdef USE_ASSERT_CHECKING
extern void verify_compact_attribute(TupleDesc, int attnum);
-#endif
/*
* Accessor for the i'th CompactAttribute element of tupdesc.
--
2.45.0
On Sat, 11 Jan 2025 at 11:27, Andrew Kane <andrew@ankane.org> wrote:
Prior to 6f3820f, extensions could be compiled with -DUSE_ASSERT_CHECKING whether or not the Postgres installation was configured with --enable-cassert (to enable at least some assertion checking). However, after 6f3820f, linking fails with `undefined symbol: verify_compact_attribute`. I'm not sure if this use case is officially supported, but it has been helpful for developing pgvector.
The attached patch fixes it, but there may be a better way to do this.
hmm, I didn't think of that scenario. I think since
verify_compact_attribute() does nothing when USE_ASSERT_CHECKING isn't
defined that we might as well define a ((void) 0) macro to avoid the
undefined symbol error. That'll avoid the useless call in your debug
builds.
I looked around for inspiration and that's what pgstat_assert_is_up()
does. Most of the other examples are static functions.
David
Attachments:
fix_verify_compact_attribute.patchapplication/octet-stream; name=fix_verify_compact_attribute.patchDownload
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index aee871b0e8..afe2adbfb6 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -159,7 +159,9 @@ TupleDescAttr(TupleDesc tupdesc, int i)
#undef TupleDescAttrAddress
#ifdef USE_ASSERT_CHECKING
-extern void verify_compact_attribute(TupleDesc, int attnum);
+extern void verify_compact_attribute(TupleDesc tupdesc, int attnum);
+#else
+#define verify_compact_attribute(tupdesc, attnum) ((void) 0)
#endif
/*
David Rowley <dgrowleyml@gmail.com> writes:
hmm, I didn't think of that scenario. I think since
verify_compact_attribute() does nothing when USE_ASSERT_CHECKING isn't
defined that we might as well define a ((void) 0) macro to avoid the
undefined symbol error. That'll avoid the useless call in your debug
builds.
No, this completely fails to address the problem. The concern is
that the extension has been compiled under USE_ASSERT_CHECKING,
so it will try to call the function. If the function's not there
in core, kaboom.
What you need to do is provide an empty no-op version of
verify_compact_attribute() in non-assert builds.
regards, tom lane
On Sat, 11 Jan 2025 at 12:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
hmm, I didn't think of that scenario. I think since
verify_compact_attribute() does nothing when USE_ASSERT_CHECKING isn't
defined that we might as well define a ((void) 0) macro to avoid the
undefined symbol error. That'll avoid the useless call in your debug
builds.No, this completely fails to address the problem. The concern is
that the extension has been compiled under USE_ASSERT_CHECKING,
so it will try to call the function. If the function's not there
in core, kaboom.
hmm, you got me confused. Maybe you missed that the extension will be
the one compiling the static inline TupleDescCompactAttr() function
and will use the macro instead?
I'm not grasping why this does not solve the problem.
David
David Rowley <dgrowleyml@gmail.com> writes:
On Sat, 11 Jan 2025 at 12:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
No, this completely fails to address the problem. The concern is
that the extension has been compiled under USE_ASSERT_CHECKING,
so it will try to call the function. If the function's not there
in core, kaboom.
hmm, you got me confused. Maybe you missed that the extension will be
the one compiling the static inline TupleDescCompactAttr() function
and will use the macro instead?
No, I don't think I missed anything. Inline function or no, the
extension will contain a call to verify_compact_attribute(),
and that won't work if the core backend doesn't have that function.
Depending on what platform you test on, the extension might have
to actually reach that function call before it fails.
regards, tom lane
I've updated the patch to make verify_compact_attribute a no-op.
The extension sets USE_ASSERT_CHECKING, which is why the macro approach
doesn't work (it won't take that path).
Also, it looks like it fails when creating the extension / loading the
shared library (on Ubuntu), not when linking (as I misstated earlier).
Attachments:
v2-0001-Restore-support-for-USE_ASSERT_CHECKING-in-extens.patchapplication/x-patch; name=v2-0001-Restore-support-for-USE_ASSERT_CHECKING-in-extens.patchDownload
From 90aab02fd882b0fd7fb8df96c80022bf64a720c5 Mon Sep 17 00:00:00 2001
From: Andrew Kane <andrew@ankane.org>
Date: Fri, 10 Jan 2025 15:41:42 -0800
Subject: [PATCH v2] Restore support for USE_ASSERT_CHECKING in extensions only
---
src/backend/access/common/tupdesc.c | 5 ++---
src/include/access/tupdesc.h | 2 --
2 files changed, 2 insertions(+), 5 deletions(-)
diff --git a/src/backend/access/common/tupdesc.c b/src/backend/access/common/tupdesc.c
index cc94074219..2e4666c469 100644
--- a/src/backend/access/common/tupdesc.c
+++ b/src/backend/access/common/tupdesc.c
@@ -118,7 +118,6 @@ populate_compact_attribute(TupleDesc tupdesc, int attnum)
populate_compact_attribute_internal(src, dst);
}
-#ifdef USE_ASSERT_CHECKING
/*
* verify_compact_attribute
* In Assert enabled builds, we verify that the CompactAttribute is
@@ -132,6 +131,7 @@ populate_compact_attribute(TupleDesc tupdesc, int attnum)
void
verify_compact_attribute(TupleDesc tupdesc, int attnum)
{
+#ifdef USE_ASSERT_CHECKING
CompactAttribute *cattr = &tupdesc->compact_attrs[attnum];
Form_pg_attribute attr = TupleDescAttr(tupdesc, attnum);
CompactAttribute tmp;
@@ -150,9 +150,8 @@ verify_compact_attribute(TupleDesc tupdesc, int attnum)
/* Check the freshly populated CompactAttribute matches the TupleDesc's */
Assert(memcmp(&tmp, cattr, sizeof(CompactAttribute)) == 0);
-}
#endif
-
+}
/*
* CreateTemplateTupleDesc
diff --git a/src/include/access/tupdesc.h b/src/include/access/tupdesc.h
index aee871b0e8..504ce22250 100644
--- a/src/include/access/tupdesc.h
+++ b/src/include/access/tupdesc.h
@@ -158,9 +158,7 @@ TupleDescAttr(TupleDesc tupdesc, int i)
#undef TupleDescAttrAddress
-#ifdef USE_ASSERT_CHECKING
extern void verify_compact_attribute(TupleDesc, int attnum);
-#endif
/*
* Accessor for the i'th CompactAttribute element of tupdesc.
--
2.45.0
On Sat, 11 Jan 2025 at 12:48, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David Rowley <dgrowleyml@gmail.com> writes:
On Sat, 11 Jan 2025 at 12:32, Tom Lane <tgl@sss.pgh.pa.us> wrote:
No, this completely fails to address the problem. The concern is
that the extension has been compiled under USE_ASSERT_CHECKING,
so it will try to call the function. If the function's not there
in core, kaboom.hmm, you got me confused. Maybe you missed that the extension will be
the one compiling the static inline TupleDescCompactAttr() function
and will use the macro instead?No, I don't think I missed anything. Inline function or no, the
extension will contain a call to verify_compact_attribute(),
and that won't work if the core backend doesn't have that function.
I think I'd only thought in terms of getting the extension to build
and failed to consider that the already built extension might be
created in builds with and without USE_ASSERT_CHECKING.
David
On Sat, 11 Jan 2025 at 12:56, Andrew Kane <andrew@ankane.org> wrote:
I've updated the patch to make verify_compact_attribute a no-op.
The extension sets USE_ASSERT_CHECKING, which is why the macro approach doesn't work (it won't take that path).
Also, it looks like it fails when creating the extension / loading the shared library (on Ubuntu), not when linking (as I misstated earlier).
I think the v2 patch looks fine then. I'll push.
David
Andrew Kane <andrew@ankane.org> writes:
I've updated the patch to make verify_compact_attribute a no-op.
The extension sets USE_ASSERT_CHECKING, which is why the macro approach
doesn't work (it won't take that path).
LGTM
Also, it looks like it fails when creating the extension / loading the
shared library (on Ubuntu), not when linking (as I misstated earlier).
I think that on macOS and perhaps a couple of other platforms, there'd
be a failure at extension link time. But yeah, most Linuxen won't
tell you till load time.
regards, tom lane
On Sat, 11 Jan 2025 at 13:05, David Rowley <dgrowleyml@gmail.com> wrote:
On Sat, 11 Jan 2025 at 12:56, Andrew Kane <andrew@ankane.org> wrote:
I've updated the patch to make verify_compact_attribute a no-op.
The extension sets USE_ASSERT_CHECKING, which is why the macro approach doesn't work (it won't take that path).
Also, it looks like it fails when creating the extension / loading the shared library (on Ubuntu), not when linking (as I misstated earlier).
I think the v2 patch looks fine then. I'll push.
Pushed. Thanks.
David