Обсуждение: Re: Restore support for USE_ASSERT_CHECKING in extensions only

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

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
(asI 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
(asI misstated earlier).
 
>
> I think the v2 patch looks fine then. I'll push.

Pushed. Thanks.

David