Horton

Thread

Commits

  1. Update obsolete comment.

  2. Clarify coding in Catalog::AddDefaultValues.

  3. Run "make reformat-dat-files".

  4. Don't require pg_class.dat to contain correct relnatts values.

  1. assert pg_class.relnatts is consistent

    Justin Pryzby <pryzby@telsasoft.com> — 2020-02-12T18:23:37Z

    Forking this thread for two tangential patches which I think are more
worthwhile than the original topic's patch.
https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com

Is there a better place to implement assertion from 0002 ?

On Fri, Feb 07, 2020 at 08:39:35AM -0600, Justin Pryzby wrote:
> From 7eea0a17e495fe13379ffd589b551f2f145f5672 Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Thu, 6 Feb 2020 21:48:13 -0600
> Subject: [PATCH v1 1/3] Update comment obsolete since b9b8831a
> 
> ---
>  src/backend/commands/cluster.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/src/backend/commands/cluster.c b/src/backend/commands/cluster.c
> index e9d7a7f..3adcbeb 100644
> --- a/src/backend/commands/cluster.c
> +++ b/src/backend/commands/cluster.c
> @@ -1539,9 +1539,9 @@ get_tables_to_cluster(MemoryContext cluster_context)
>  
>  	/*
>  	 * Get all indexes that have indisclustered set and are owned by
> -	 * appropriate user. System relations or nailed-in relations cannot ever
> -	 * have indisclustered set, because CLUSTER will refuse to set it when
> -	 * called with one of them as argument.
> +	 * appropriate user. Shared relations cannot ever have indisclustered
> +	 * set, because CLUSTER will refuse to set it when called with one as
> +	 * an argument.
>  	 */
>  	indRelation = table_open(IndexRelationId, AccessShareLock);
>  	ScanKeyInit(&entry,
> -- 
> 2.7.4
> 

> From 4777be522a7aa8b8c77b13f765cbd02043438f2a Mon Sep 17 00:00:00 2001
> From: Justin Pryzby <pryzbyj@telsasoft.com>
> Date: Fri, 7 Feb 2020 08:12:50 -0600
> Subject: [PATCH v1 2/3] Give developer a helpful kick in the pants if they
>  change natts in one place but not another
> 
> ---
>  src/backend/bootstrap/bootstrap.c | 23 +++++++++++++++++++++++
>  1 file changed, 23 insertions(+)
> 
> diff --git a/src/backend/bootstrap/bootstrap.c b/src/backend/bootstrap/bootstrap.c
> index bfc629c..d5e1888 100644
> --- a/src/backend/bootstrap/bootstrap.c
> +++ b/src/backend/bootstrap/bootstrap.c
> @@ -25,7 +25,9 @@
>  #include "access/xlog_internal.h"
>  #include "bootstrap/bootstrap.h"
>  #include "catalog/index.h"
> +#include "catalog/pg_class.h"
>  #include "catalog/pg_collation.h"
> +#include "catalog/pg_proc.h"
>  #include "catalog/pg_type.h"
>  #include "common/link-canary.h"
>  #include "libpq/pqsignal.h"
> @@ -49,6 +51,7 @@
>  #include "utils/ps_status.h"
>  #include "utils/rel.h"
>  #include "utils/relmapper.h"
> +#include "utils/syscache.h"
>  
>  uint32		bootstrap_data_checksum_version = 0;	/* No checksum */
>  
> @@ -602,6 +605,26 @@ boot_openrel(char *relname)
>  	TableScanDesc scan;
>  	HeapTuple	tup;
>  
> +	/* Check that pg_class data is consistent now, rather than failing obscurely later */
> +	struct { Oid oid; int natts; }
> +		checknatts[] = {
> +		{RelationRelationId, Natts_pg_class,},
> +		{TypeRelationId, Natts_pg_type,},
> +		{AttributeRelationId, Natts_pg_attribute,},
> +		{ProcedureRelationId, Natts_pg_proc,},
> +	};
> +
> +	for (int i=0; i<lengthof(checknatts); ++i) {
> +		Form_pg_class	classForm;
> +		HeapTuple	tuple;
> +		tuple = SearchSysCache1(RELOID, ObjectIdGetDatum(checknatts[i].oid));
> +		if (!HeapTupleIsValid(tuple))
> +			elog(ERROR, "cache lookup failed for relation %u", checknatts[i].oid);
> +		classForm = (Form_pg_class) GETSTRUCT(tuple);
> +		Assert(checknatts[i].natts == classForm->relnatts);
> +		ReleaseSysCache(tuple);
> +	}
> +
>  	if (strlen(relname) >= NAMEDATALEN)
>  		relname[NAMEDATALEN - 1] = '\0';
>  
> -- 
> 2.7.4
>

  2. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-13T07:51:01Z

    On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> Forking this thread for two tangential patches which I think are more
> worthwhile than the original topic's patch.
> https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com
>
> Is there a better place to implement assertion from 0002 ?

I would think the answer to that would be related to the answer of why
you think we need this assert in the first place?

I know I have made the mistake of not updating relnatts when I added
relispartition, etc. to pg_class, only to be bitten by it in the form
of seemingly random errors/crashes.  Is that why?

Thanks,
Amit

  3. Re: assert pg_class.relnatts is consistent

    Michael Paquier <michael@paquier.xyz> — 2020-02-13T08:10:12Z

    On Thu, Feb 13, 2020 at 04:51:01PM +0900, Amit Langote wrote:
> I would think the answer to that would be related to the answer of why
> you think we need this assert in the first place?

Taking this thread independently, and even after reading the thread
mentioned upthread, I still don't quite understand why this change
could be a good thing and in which cases it actually helps.  The code
includes no comments and the commit log says nothing either, so it is
hard to follow what you are thinking here even if you are splitting
the effort across multiple thread.  Please note that the style of the
code is not project-like, so you should try to indent it.  And why
does it matter to check this portion of the catalogs?  Also, such
checks are not really needed in non-assert builds, if actually
needed.
--
Michael

  4. Re: assert pg_class.relnatts is consistent

    Justin Pryzby <pryzby@telsasoft.com> — 2020-02-13T08:11:45Z

    On Thu, Feb 13, 2020 at 04:51:01PM +0900, Amit Langote wrote:
> On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Forking this thread for two tangential patches which I think are more
> > worthwhile than the original topic's patch.
> > https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com
> >
> > Is there a better place to implement assertion from 0002 ?
> 
> I would think the answer to that would be related to the answer of why
> you think we need this assert in the first place?
> 
> I know I have made the mistake of not updating relnatts when I added
> relispartition, etc. to pg_class, only to be bitten by it in the form
> of seemingly random errors/crashes.  Is that why?

Right.  If adding or removing a column from pg_class (or others) it's necessary
not only to add the column in the .h file, and update references like Anum_*,
but also to update that catalog's own pg_class.relnatts in pg_class.dat.

On the other thead, Alvaro agreed it might be worth experimenting with moving
"indisclustered" from boolean in pg_index to an Oid in pg_class.  There's not
many references to it, so I was able to make most of the necessary changes
within an hour .. but spent some multiple of that tracing the crash in initdb,
which I would prefer to have failed less obscurely.

-- 
Justin

  5. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-13T08:12:48Z

    On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Thu, Feb 13, 2020 at 3:23 AM Justin Pryzby <pryzby@telsasoft.com> wrote:
> > Forking this thread for two tangential patches which I think are more
> > worthwhile than the original topic's patch.
> > https://www.postgresql.org/message-id/20200207143935.GP403%40telsasoft.com
> >
> > Is there a better place to implement assertion from 0002 ?
>
> I would think the answer to that would be related to the answer of why
> you think we need this assert in the first place?
>
> I know I have made the mistake of not updating relnatts when I added
> relispartition, etc. to pg_class, only to be bitten by it in the form
> of seemingly random errors/crashes.  Is that why?

Sorry for not having read the patch properly.

> +     /* Check that pg_class data is consistent now, rather than failing obscurely later */

That seems to be it.

Thanks,
Amit

  6. Re: assert pg_class.relnatts is consistent

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-13T16:04:03Z

    Amit Langote <amitlangote09@gmail.com> writes:
> On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
>> I know I have made the mistake of not updating relnatts when I added
>> relispartition, etc. to pg_class, only to be bitten by it in the form
>> of seemingly random errors/crashes.  Is that why?

> Sorry for not having read the patch properly.
>> +     /* Check that pg_class data is consistent now, rather than failing obscurely later */
> That seems to be it.

I've been burnt by this too :-(.  However, I think this patch is
completely the wrong way to go about improving this.  What we should
be doing, now that we have all that perl code generating postgres.bki,
is eliminating the problem at the source.  That is, drop the hand-coded
relnatts values from pg_class.dat altogether, and let the perl code fill
it in --- compare the handling of pg_proc.pronargs for instance.

(While we're at it, an awful lot of the bulk of pg_class.dat could be
replaced by BKI_DEFAULT() entries in pg_class.h, though I'm less sure
that that's much of an improvement.  I think we intentionally didn't
bother when we put in the BKI_DEFAULT support, reasoning that there
were too few pg_class.dat entries to bother.)

			regards, tom lane

  7. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-14T05:58:37Z

    On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
>
> Amit Langote <amitlangote09@gmail.com> writes:
> > On Thu, Feb 13, 2020 at 4:51 PM Amit Langote <amitlangote09@gmail.com> wrote:
> >> I know I have made the mistake of not updating relnatts when I added
> >> relispartition, etc. to pg_class, only to be bitten by it in the form
> >> of seemingly random errors/crashes.  Is that why?
>
> > Sorry for not having read the patch properly.
> >> +     /* Check that pg_class data is consistent now, rather than failing obscurely later */
> > That seems to be it.
>
> I've been burnt by this too :-(.  However, I think this patch is
> completely the wrong way to go about improving this.  What we should
> be doing, now that we have all that perl code generating postgres.bki,
> is eliminating the problem at the source.  That is, drop the hand-coded
> relnatts values from pg_class.dat altogether, and let the perl code fill
> it in --- compare the handling of pg_proc.pronargs for instance.

I can't write Perl myself (maybe Justin), but +1 to this idea.

Thanks,
Amit

  8. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-14T09:00:05Z

    On Fri, Feb 14, 2020 at 2:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > I've been burnt by this too :-(.  However, I think this patch is
> > completely the wrong way to go about improving this.  What we should
> > be doing, now that we have all that perl code generating postgres.bki,
> > is eliminating the problem at the source.  That is, drop the hand-coded
> > relnatts values from pg_class.dat altogether, and let the perl code fill
> > it in --- compare the handling of pg_proc.pronargs for instance.
>
> I can't write Perl myself (maybe Justin), but +1 to this idea.

I tried and think it works but not sure if that's good Perl
programming.  See the attached.

Thanks,
Amit

  9. Re: assert pg_class.relnatts is consistent

    Michael Paquier <michael@paquier.xyz> — 2020-02-14T09:47:23Z

    On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote:
> On Fri, Feb 14, 2020 at 2:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > I've been burnt by this too :-(.  However, I think this patch is
> > > completely the wrong way to go about improving this.  What we should
> > > be doing, now that we have all that perl code generating postgres.bki,
> > > is eliminating the problem at the source.  That is, drop the hand-coded
> > > relnatts values from pg_class.dat altogether, and let the perl code fill
> > > it in --- compare the handling of pg_proc.pronargs for instance.
> >
> > I can't write Perl myself (maybe Justin), but +1 to this idea.
> 
> I tried and think it works but not sure if that's good Perl
> programming.  See the attached.

I quite like what you have here.  Please note that this comment in
genbki.pl is incorrect regarding relnatts (the last part could just be
deleted):
# Note: only bootstrap catalogs, ie those marked BKI_BOOTSTRAP, need to
# have entries here.  Be sure that the OIDs listed here match those given in
# their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are
# correct.
--
Michael

  10. Re: assert pg_class.relnatts is consistent

    John Naylor <john.naylor@2ndquadrant.com> — 2020-02-14T09:50:47Z

    On Fri, Feb 14, 2020 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> I tried and think it works but not sure if that's good Perl
> programming.  See the attached.

Hi Amit,
I took this for a spin -- I just have a couple comments.

+ elsif ($attname eq 'relnatts')
+ {
+ ;
+ }

With your patch, I get this when running
src/include/catalog/reformat_dat_file.pl:

strip_default_values: pg_class.relnatts undefined

Rather than adding this one-off case to AddDefaultValues and then
another special case to strip_default_values, maybe it would be better
to just add a placeholder BKI_DEFAULT(0) to pg_class.h, with a comment
that it's just a placeholder.


+ if ($catname eq "pg_class" && $attname eq "relnatts")
+ {
+ $bki_values{$attname} = $catalog_ncols{$bki_values{relname}};
+ }
+

You could avoid the name/attr checks if you do it while building the
pg_class lookup table, like this:

 foreach my $row (@{ $catalog_data{pg_class} })
 {
  $classoids{ $row->{relname} } = $row->{oid};
+
+ # Also fill in correct value for relnatts.
+ $row->{relnatts} = $catalog_ncols{ $row->{relname} };
 }

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  11. Re: assert pg_class.relnatts is consistent

    John Naylor <john.naylor@2ndquadrant.com> — 2020-02-14T10:39:59Z

    I wrote:
> + elsif ($attname eq 'relnatts')
> + {
> + ;
> + }
>
> With your patch, I get this when running
> src/include/catalog/reformat_dat_file.pl:
>
> strip_default_values: pg_class.relnatts undefined
>
> Rather than adding this one-off case to AddDefaultValues and then
> another special case to strip_default_values, maybe it would be better
> to just add a placeholder BKI_DEFAULT(0) to pg_class.h, with a comment
> that it's just a placeholder.

One possible objection to what I wrote above is that it adds a
different kind of special case, but in a sneaky way. Perhaps it would
be more principled to treat it the same as oid after all. If we do
that, it would help to add a comment that we can't treat relnatts like
pronangs, since we need more information than what's in each pg_class
row.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  12. Re: assert pg_class.relnatts is consistent

    John Naylor <john.naylor@2ndquadrant.com> — 2020-02-14T10:42:32Z

    > pronangs, since we need more information than what's in each pg_class

Sigh, and of course I met pg_proc.pronargs.

-- 
John Naylor                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  13. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-14T12:44:19Z

    On Fri, Feb 14, 2020 at 6:47 PM Michael Paquier <michael@paquier.xyz> wrote:
> On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote:
> > On Fri, Feb 14, 2020 at 2:58 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > > On Fri, Feb 14, 2020 at 1:04 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> > > > I've been burnt by this too :-(.  However, I think this patch is
> > > > completely the wrong way to go about improving this.  What we should
> > > > be doing, now that we have all that perl code generating postgres.bki,
> > > > is eliminating the problem at the source.  That is, drop the hand-coded
> > > > relnatts values from pg_class.dat altogether, and let the perl code fill
> > > > it in --- compare the handling of pg_proc.pronargs for instance.
> > >
> > > I can't write Perl myself (maybe Justin), but +1 to this idea.
> >
> > I tried and think it works but not sure if that's good Perl
> > programming.  See the attached.
>
> I quite like what you have here.  Please note that this comment in
> genbki.pl is incorrect regarding relnatts (the last part could just be
> deleted):
> # Note: only bootstrap catalogs, ie those marked BKI_BOOTSTRAP, need to
> # have entries here.  Be sure that the OIDs listed here match those given in
> # their CATALOG and BKI_ROWTYPE_OID macros, and that the relnatts values are
> # correct.

You're right, although this comment is in pg_class.dat.

Thanks,
Amit

  14. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-14T14:22:05Z

    Hi John,

On Fri, Feb 14, 2020 at 6:50 PM John Naylor <john.naylor@2ndquadrant.com> wrote:
> On Fri, Feb 14, 2020 at 5:00 PM Amit Langote <amitlangote09@gmail.com> wrote:
> > I tried and think it works but not sure if that's good Perl
> > programming.  See the attached.
>
> Hi Amit,
> I took this for a spin -- I just have a couple comments.

Thanks for chiming in.

> + elsif ($attname eq 'relnatts')
> + {
> + ;
> + }
>
> With your patch, I get this when running
> src/include/catalog/reformat_dat_file.pl:
>
> strip_default_values: pg_class.relnatts undefined

I think I have fixed this in the attached.

> + if ($catname eq "pg_class" && $attname eq "relnatts")
> + {
> + $bki_values{$attname} = $catalog_ncols{$bki_values{relname}};
> + }
> +
>
> You could avoid the name/attr checks if you do it while building the
> pg_class lookup table, like this:
>
>  foreach my $row (@{ $catalog_data{pg_class} })
>  {
>   $classoids{ $row->{relname} } = $row->{oid};
> +
> + # Also fill in correct value for relnatts.
> + $row->{relnatts} = $catalog_ncols{ $row->{relname} };
>  }

Did this too.  Attached updated patch, which also addresses Michael's comment.

I'm still trying to understand your comment about using placeholder
BKI_DEFAULT...

Thanks,
Amit

  15. Re: assert pg_class.relnatts is consistent

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-14T15:13:01Z

    I propose this more concise coding for AddDefaultValues,

	# Now fill in defaults, and note any columns that remain undefined.
	foreach my $column (@$schema)
	{
		my $attname = $column->{name};
		my $atttype = $column->{type};

		# Skip if a value already exists
		next if defined $row->{$attname};

		# 'oid' and 'relnatts' are special cases. Ignore.
		next if $attname eq 'oid';
		next if $attname eq 'relnatts';

		# This column has a default value.  Fill it in.
		if (defined $column->{default})
		{
			$row->{$attname} = $column->{default};
			next;
		}

		# Failed to find a value.
		push @missing_fields, $attname;
	}

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  16. Re: assert pg_class.relnatts is consistent

    Alvaro Herrera <alvherre@2ndquadrant.com> — 2020-02-14T15:29:26Z

    On 2020-Feb-14, John Naylor wrote:

> One possible objection to what I wrote above is that it adds a
> different kind of special case, but in a sneaky way. Perhaps it would
> be more principled to treat it the same as oid after all. If we do
> that, it would help to add a comment that we can't treat relnatts like
> pronangs, since we need more information than what's in each pg_class
> row.

How about something like this? (untested)

		# oids are a special case; ignore
		next if $attname eq 'oid';
		# pg_class.relnatts is computed from pg_attribute rows; ignore
		next if $catname eq 'pg_class' and $attname eq 'relnatts';

		# Raise error unless a value exists.
		die "strip_default_values: $catname.$attname undefined\n"
		  if !defined $row->{$attname};

-- 
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

  17. Re: assert pg_class.relnatts is consistent

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-14T19:00:56Z

    Alvaro Herrera <alvherre@2ndquadrant.com> writes:
> On 2020-Feb-14, John Naylor wrote:
>> One possible objection to what I wrote above is that it adds a
>> different kind of special case, but in a sneaky way. Perhaps it would
>> be more principled to treat it the same as oid after all. If we do
>> that, it would help to add a comment that we can't treat relnatts like
>> pronangs, since we need more information than what's in each pg_class
>> row.

> How about something like this? (untested)

I think John's idea of setting a dummy BKI_DEFAULT value is better,
as that means the only code that has to worry about this directly
is the code that's actually filling in relnatts.  As far as said
code goes, we don't need an additional global variable when we can
just look in the $catalogs data structure; and I'm not a fan of
cramming this into the OID-assignment logic just to save a loop.
So that leads me to the attached.

(I agree with Alvaro's thought of shortening AddDefaultValues,
but didn't do that here.)

			regards, tom lane

  18. Re: assert pg_class.relnatts is consistent

    Tom Lane <tgl@sss.pgh.pa.us> — 2020-02-15T20:25:55Z

    I wrote:
> So that leads me to the attached.
> ...
> (I agree with Alvaro's thought of shortening AddDefaultValues,
> but didn't do that here.)

Pushed both of those.  I also did something with the stale comment
that Justin referred to in the initial message (it wasn't really
good practice to try to deal with both things in one thread).

I think we're done here, though maybe the difficulty of finding a clean
way to get genbki.pl to do this suggests that AddDefaultValues needs
to be redesigned.  Not sure what that'd look like.

			regards, tom lane

  19. Re: assert pg_class.relnatts is consistent

    Amit Langote <amitlangote09@gmail.com> — 2020-02-17T04:25:05Z

    On Sun, Feb 16, 2020 at 5:25 AM Tom Lane <tgl@sss.pgh.pa.us> wrote:
> I wrote:
> > So that leads me to the attached.
> > ...
> > (I agree with Alvaro's thought of shortening AddDefaultValues,
> > but didn't do that here.)
>
> Pushed both of those.

Thank you.

It's amazing to see how simple bootstrapping has now become thanks to
the work you guys have done recently.

Thanks,
Amit

  20. Re: assert pg_class.relnatts is consistent

    Justin Pryzby <pryzby@telsasoft.com> — 2020-02-17T04:44:14Z

    On Mon, Feb 17, 2020 at 01:25:05PM +0900, Amit Langote wrote:
> > Pushed both of those.
> 
> Thank you.
> 
> It's amazing to see how simple bootstrapping has now become thanks to
> the work you guys have done recently.

On Fri, Feb 14, 2020 at 06:00:05PM +0900, Amit Langote wrote:
> > I can't write Perl myself (maybe Justin), but +1 to this idea.
> 
> I tried and think it works but not sure if that's good Perl
> programming.  See the attached.

And thanks for picking up perl so I didn't have to remember what I ever knew.

-- 
Justin