Horton

Thread

Commits

  1. Make verify_compact_attribute available in non-assert builds

  1. Restore support for USE_ASSERT_CHECKING in extensions only

    Andrew Kane <andrew@ankane.org> — 2025-01-10T22:26:50Z

    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.

  2. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    David Rowley <dgrowleyml@gmail.com> — 2025-01-10T23:17:23Z

    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

  3. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-10T23:32:03Z

    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

  4. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    David Rowley <dgrowleyml@gmail.com> — 2025-01-10T23:40:46Z

    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

  5. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-10T23:48:08Z

    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

  6. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    Andrew Kane <andrew@ankane.org> — 2025-01-10T23:55:53Z

    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).

  7. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    David Rowley <dgrowleyml@gmail.com> — 2025-01-10T23:56:29Z

    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

  8. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    David Rowley <dgrowleyml@gmail.com> — 2025-01-11T00:05:07Z

    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

  9. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    Tom Lane <tgl@sss.pgh.pa.us> — 2025-01-11T00:15:09Z

    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

  10. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    David Rowley <dgrowleyml@gmail.com> — 2025-01-11T00:46:51Z

    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

  11. Re: Restore support for USE_ASSERT_CHECKING in extensions only

    Andrew Kane <andrew@ankane.org> — 2025-01-11T01:00:50Z

    Thank you both!