Re: some namespace.c refactoring - Mailing list pgsql-hackers

From Alvaro Herrera
Subject Re: some namespace.c refactoring
Date
Msg-id 20230215180456.2aoayhcgbc2fgy3o@alvherre.pgsql
Whole thread Raw
In response to Re: some namespace.c refactoring  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: some namespace.c refactoring
List pgsql-hackers
On 2023-Feb-15, Tom Lane wrote:

> Peter Eisentraut <peter.eisentraut@enterprisedb.com> writes:
> > Here are two patches that refactor the mostly repetitive "${object} is 
> > visible" and get_${object}_oid() functions in namespace.c.  This uses 
> > the functions in objectaddress.c to look up the appropriate per-catalog 
> > system caches and attribute numbers, similar to other refactoring 
> > patches I have posted recently.
> 
> This does not look like a simple refactoring patch to me.  I have
> very serious concerns first about whether it even preserves the
> existing semantics, and second about whether there is a performance
> penalty.

I suppose there are two possible questionable angles from a performance
POV:

1. the new code uses get_object_property_data() when previously there
was a straight dereference after casting to the right struct type.  So
how expensive is that?  I think the answer to that is not great, because
it does a linear array scan on ObjectProperty.  Maybe we need a better
answer.

2. other accesses to the data are done using SysCacheGetAttr instead of
straight struct access dereferences.  I expect that most of the fields
being accessed have attcacheoff set, which allows pretty fast access to
the field in question, so it's not *that* bad.  (For cases where
attcacheoff is not set, then the original code would also have to deform
the tuple.)  Still, it's going to have nontrivial impact in any
microbenchmarking.

That said, I think most of this code is invoked for DDL, where
performance is not so critical; probably just fixing
get_object_property_data to not be so naïve would suffice.

Queries are another matter.  I can't think of a way to determine which
of these paths are used for queries, so that we can optimize more (IOW:
just plain not rewrite those); manually going through the callers seems
a bit laborious, but perhaps doable.

-- 
Álvaro Herrera        Breisgau, Deutschland  —  https://www.EnterpriseDB.com/
Thou shalt check the array bounds of all strings (indeed, all arrays), for
surely where thou typest "foo" someone someday shall type
"supercalifragilisticexpialidocious" (5th Commandment for C programmers)



pgsql-hackers by date:

Previous
From: Nathan Bossart
Date:
Subject: Re: We shouldn't signal process groups with SIGQUIT
Next
From: Andres Freund
Date:
Subject: Re: We shouldn't signal process groups with SIGQUIT