Thread

Commits

Move catalog index declarations
- bdc4edbea6fc 14.0 landed
Move catalog toast table declarations
- b4c9695e79f7 14.0 landed

Move catalog toast table and index declarations

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-10-22T10:21:11Z

As discussed in [0], here are patches to move the system catalog toast 
table and index declarations from catalog/toasting.h and 
catalog/indexing.h to the respective parent tables' catalog/pg_*.h 
files.  I think it's clearly better to have everything together like this.

The original reason for having it split was that the old genbki system 
produced the output in the order of the catalog files it read, so all 
the toasting and indexing stuff needed to come separately.  But this is 
no longer the case.

The resulting postgres.bki file has some ordering differences *within* 
the toast and index groups, but these should not be significant.  (It's 
basically done in the order of the parent catalogs now rather than 
whatever the old file order was.)

In this patch set, I moved the DECLARE_* lines as is.  In the discussion 
[0] some ideas were floated for altering or tweaking these things, but I 
suggest that can be undertaken as a separate patch set.


[0]: 
https://www.postgresql.org/message-id/20201006201549.em2meighuapttl7n%40alap3.anarazel.de

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Move catalog toast table and index declarations

John Naylor <john.naylor@enterprisedb.com> — 2020-10-24T13:23:56Z

On Thu, Oct 22, 2020 at 6:21 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:
> [v1]

Hi Peter,

This part created a syntax error:

--- a/src/include/catalog/unused_oids
+++ b/src/include/catalog/unused_oids
@@ -28,7 +28,7 @@ chdir $FindBin::RealBin or die "could not cd to
$FindBin::RealBin: $!\n";
 use lib "$FindBin::RealBin/../../backend/catalog/";
 use Catalog;

-my @input_files = (glob("pg_*.h"), qw(indexing.h));
+my @input_files = (glob("pg_*.h");

Style: In genbki.h, "extern int no_such_variable" is now out of place.
Also, the old comments like "The macro definitions are just to keep the C
compiler from spitting up." are now redundant in their new setting.

The rest looks good to me. unused_oids (once fixed), duplicate_oids, and
renumber_oids.pl seem to work fine.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Move catalog toast table and index declarations

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-10-27T11:42:58Z

On 2020-10-24 15:23, John Naylor wrote:
> This part created a syntax error:
> 
> --- a/src/include/catalog/unused_oids
> +++ b/src/include/catalog/unused_oids
> @@ -28,7 +28,7 @@ chdir $FindBin::RealBin or die "could not cd to 
> $FindBin::RealBin: $!\n";
>   use lib "$FindBin::RealBin/../../backend/catalog/";
>   use Catalog;
> 
> -my @input_files = (glob("pg_*.h"), qw(indexing.h));
> +my @input_files = (glob("pg_*.h");

OK, fixing that.

> Style: In genbki.h, "extern int no_such_variable" is now out of place. 
> Also, the old comments like "The macro definitions are just to keep the 
> C compiler from spitting up." are now redundant in their new setting.

Hmm, I don't really see what's wrong there.  Do you mean the macro 
definitions should be different, or the comments are wrong, or something 
else?

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Move catalog toast table and index declarations

John Naylor <john.naylor@enterprisedb.com> — 2020-10-27T12:12:11Z

On Tue, Oct 27, 2020 at 7:43 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 2020-10-24 15:23, John Naylor wrote:
> > Style: In genbki.h, "extern int no_such_variable" is now out of place.
> > Also, the old comments like "The macro definitions are just to keep the
> > C compiler from spitting up." are now redundant in their new setting.
>
> Hmm, I don't really see what's wrong there.  Do you mean the macro
> definitions should be different, or the comments are wrong, or something
> else?
>

There's nothing wrong; it's just a minor point of consistency. For the
first part, I mean defined symbols in this file that are invisible to the C
compiler are written

#define SOMETHING()

If some are written

#define SOMETHING() extern int no_such_variable

I imagine some future reader will wonder why there's a difference.

As for the comments, the entire file is for things meant for scripts to
read, but have to be put in macro form to be invisible to the compiler. The
header comment has

"genbki.h defines CATALOG(), BKI_BOOTSTRAP and related macros
 * so that the catalog header files can be read by the C compiler."

I'm just saying we don't need to carry over the comments I mentioned from
the toasting/indexing headers that were specially for those macros.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Move catalog toast table and index declarations

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-11-05T08:24:08Z

On 2020-10-27 13:12, John Naylor wrote:
> There's nothing wrong; it's just a minor point of consistency. For the 
> first part, I mean defined symbols in this file that are invisible to 
> the C compiler are written
> 
> #define SOMETHING()
> 
> If some are written
> 
> #define SOMETHING() extern int no_such_variable
> 
> I imagine some future reader will wonder why there's a difference.

The difference is that CATALOG() is followed in actual use by something like

     { ... } FormData_pg_attribute;

so it becomes a valid C statement.  For DECLARE_INDEX() etc., we need to 
do something else to make it valid.  I guess this could be explained in 
more detail (as I'm attempting in this email), but this isn't materially 
changed by this patch.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Move catalog toast table and index declarations

John Naylor <john.naylor@enterprisedb.com> — 2020-11-05T11:59:03Z

On Thu, Nov 5, 2020 at 4:24 AM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 2020-10-27 13:12, John Naylor wrote:
> > There's nothing wrong; it's just a minor point of consistency. For the
> > first part, I mean defined symbols in this file that are invisible to
> > the C compiler are written
> >
> > #define SOMETHING()
> >
> > If some are written
> >
> > #define SOMETHING() extern int no_such_variable
> >
> > I imagine some future reader will wonder why there's a difference.
>
> The difference is that CATALOG() is followed in actual use by something
> like
>
>      { ... } FormData_pg_attribute;
>
> so it becomes a valid C statement.  For DECLARE_INDEX() etc., we need to
> do something else to make it valid.  I guess this could be explained in
> more detail (as I'm attempting in this email), but this isn't materially
> changed by this patch.
>

I think we're talking past eachother. Here's a concrete example:

#define BKI_ROWTYPE_OID(oid,oidmacro)
#define DECLARE_TOAST(name,toastoid,indexoid) extern int no_such_variable

I understand these to be functionally equivalent as far as what the C
compiler sees. If not, I'd be curious to know what the difference is. I was
thinking this is just a random style difference, and if so, they should be
the same now that they're in the same file together:

#define BKI_ROWTYPE_OID(oid,oidmacro)
#define DECLARE_TOAST(name,toastoid,indexoid)

And yes, this doesn't materially change the patch, it's just nitpicking :-)
. Materially, I believe it's fine.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Move catalog toast table and index declarations

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-11-05T18:20:06Z

On 2020-11-05 12:59, John Naylor wrote:
> I think we're talking past eachother. Here's a concrete example:
> 
> #define BKI_ROWTYPE_OID(oid,oidmacro)
> #define DECLARE_TOAST(name,toastoid,indexoid) extern int no_such_variable
> 
> I understand these to be functionally equivalent as far as what the C 
> compiler sees.

The issue is that you can't have a bare semicolon at the top level of a 
C compilation unit, at least on some compilers.  So doing

#define FOO(stuff) /*empty*/

and then

FOO(123);

won't work.  You need to fill the definition of FOO with some stuff to 
make it valid.

BKI_ROWTYPE_OID on the other hand is not used at the top level like 
this, so it can be defined to empty.

-- 
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Move catalog toast table and index declarations

John Naylor <john.naylor@enterprisedb.com> — 2020-11-06T13:00:52Z

On Thu, Nov 5, 2020 at 2:20 PM Peter Eisentraut <
peter.eisentraut@2ndquadrant.com> wrote:

> On 2020-11-05 12:59, John Naylor wrote:
> > I think we're talking past eachother. Here's a concrete example:
> >
> > #define BKI_ROWTYPE_OID(oid,oidmacro)
> > #define DECLARE_TOAST(name,toastoid,indexoid) extern int no_such_variable
> >
> > I understand these to be functionally equivalent as far as what the C
> > compiler sees.
>
> The issue is that you can't have a bare semicolon at the top level of a
> C compilation unit, at least on some compilers.  So doing
>
> #define FOO(stuff) /*empty*/
>
> and then
>
> FOO(123);
>
> won't work.  You need to fill the definition of FOO with some stuff to
> make it valid.
>
> BKI_ROWTYPE_OID on the other hand is not used at the top level like
> this, so it can be defined to empty.
>

Thank you for the explanation.

-- 
John Naylor
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

Re: Move catalog toast table and index declarations

Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2020-11-07T11:33:51Z

On 2020-11-05 12:59, John Naylor wrote:
> And yes, this doesn't materially change the patch, it's just nitpicking 
> :-) . Materially, I believe it's fine.

OK, committed.

-- 
Peter Eisentraut
2ndQuadrant, an EDB company
https://www.2ndquadrant.com/