Subject: Re: Reducing connection overhead in pg_upgrade compat check phase

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 6 Jul 2023 17:58:33 +0200

> On 4 Jul 2023, at 21:08, Nathan Bossart <[email-masked]> wrote:
> 
> I put together a rebased version of the patch for cfbot.

Thanks for doing that, much appreciated!  I was busy looking at other peoples
patches and hadn't gotten to my own yet =)

> On 13 Mar 2023, at 19:21, Nathan Bossart <[email-masked]> wrote:

> I noticed that git-am complained when I applied the patch:
> 
>    Applying: pg_upgrade: run all data type checks per connection
>    .git/rebase-apply/patch:1023: new blank line at EOF.
>    +
>    warning: 1 line adds whitespace errors.

Fixed.

> +				for (int rowno = 0; rowno < ntups; rowno++)
> +				{
> +					found = true;
> 
> It looks like "found" is set unconditionally a few lines above, so I think
> this is redundant.

Correct, this must've been a leftover from a previous coding that changed.
Removed.

> Also, I think it would be worth breaking check_for_data_types_usage() into
> a few separate functions (or doing some other similar refactoring) to
> improve readability.  At this point, the function is quite lengthy, and I
> count 6 levels of indentation at some lines.

It it is pretty big for sure, but it's also IMHO not terribly complicated as
it's not really performing any hard to follow logic.

I have no issues refactoring it, but trying my hand at I was only making (what
I consider) less readable code by having to jump around so I consider it a
failure.  If you have any suggestions, I would be more than happy to review and
incorporate those though.

Attached is a v5 with the above fixes and a pgindenting to fix up a few runaway
comments and indentations.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Sat, 8 Jul 2023 14:43:54 -0700

Thanks for the new patch.

> incorporate those though.

I don't have a strong opinion about this.

+				for (int rowno = 0; rowno < ntups; rowno++)
+				{
+					if (script == NULL && (script = fopen_priv(output_path, "a")) == NULL)
+						pg_fatal("could not open file \"%s\": %s",
+								 output_path,
+								 strerror(errno));
+					if (!db_used)
+					{
+						fprintf(script, "In database: %s\n", active_db->db_name);
+						db_used = true;
+					}

Since "script" will be NULL and "db_used" will be false in the first
iteration of the loop, couldn't we move this stuff to before the loop?

+					fprintf(script, " %s.%s.%s\n",
+							PQgetvalue(res, rowno, i_nspname),
+							PQgetvalue(res, rowno, i_relname),
+							PQgetvalue(res, rowno, i_attname));

nitpick: І think the current code has two spaces at the beginning of this
format string.  Did you mean to remove one of them?

+				if (script)
+				{
+					fclose(script);
+					script = NULL;
+				}

Won't "script" always be initialized here?  If I'm following this code
correctly, I think everything except the fclose() can be removed.

+			cur_check++;

I think this is unnecessary since we assign "cur_check" at the beginning of
every loop iteration.  I see two of these.

+static int	n_data_types_usage_checks = 7;

Can we determine this programmatically so that folks don't need to remember
to update it?

+	/* Prepare an array to store the results of checks in */
+	results = pg_malloc(sizeof(bool) * n_data_types_usage_checks);
+	memset(results, true, sizeof(*results));

IMHO it's a little strange that this is initialized to all "true", only
because I think most other Postgres code does the opposite.

+bool
+check_for_aclitem_data_type_usage(ClusterInfo *cluster)

Do you think we should rename these functions to something like
"should_check_for_*"?  They don't actually do the check, they just tell you
whether you should based on the version.  In fact, I wonder if we could
just add the versions directly to data_types_usage_checks so that we don't
need the separate hook functions.

-- 
Nathan Bossart
Amazon Web Services: https://aws.amazon.com

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 10 Jul 2023 16:43:23 +0200

> On 8 Jul 2023, at 23:43, Nathan Bossart <[email-masked]> wrote:

Thanks for reviewing!

> iteration of the loop, couldn't we move this stuff to before the loop?

We could.  It's done this way to match how all the other checks are performing
the inner loop for consistency.  I think being consistent is better than micro
optimizing in non-hot codepaths even though it adds some redundancy.

> format string.  Did you mean to remove one of them?

Nice catch, I did not. Fixed.

> correctly, I think everything except the fclose() can be removed.

You are right that this check is superfluous.  This is again an artifact of
modelling the code around how the other checks work for consistency.  At least
I think that's a good characteristic of the code.

> every loop iteration.  I see two of these.

Right, this is a pointless leftover from a previous version which used a
while() loop, and I had missed removing them.  Fixed.

> Can we determine this programmatically so that folks don't need to remember

Fair point, I've added a counter loop to the beginning of the check function to
calculate it.

> because I think most other Postgres code does the opposite.

Agreed, but it made for a less contrived codepath in knowing when an error has
been seen already, to avoid duplicate error output, so I think it's worth it.

> whether you should based on the version.

I've been pondering that too, and did a rename now along with moving them all
to a single place as well as changing the comments to make it clearer.

> In fact, I wonder if we could just add the versions directly to
> data_types_usage_checks so that we don't need the separate hook functions.

We could, but it would be sort of contrived I think since some check <= and
some == while some check the catversion as well (and new ones may have other
variants.  I think this is the least paint-ourselves-in-a-corner version, if we
feel it's needlessly complicated and no other variants are added we can revisit
this.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Mon, 10 Jul 2023 16:09:07 -0700

On Mon, Jul 10, 2023 at 04:43:23PM +0200, Daniel Gustafsson wrote:
> [ ... ] 

I can't say I agree with this, but I'm not going to hold up the patch over
it.  FWIW I was looking at this more from a code simplification/readability
standpoint.

> Fair point, I've added a counter loop to the beginning of the check function to

+	/* Gather number of checks to perform */
+	while (tmp->status != NULL)
+		n_data_types_usage_checks++;

I think we need to tmp++ somewhere here.

> feel it's needlessly complicated and no other variants are added we can revisit

Makes sense.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 11 Jul 2023 01:26:33 +0200

> On 11 Jul 2023, at 01:09, Nathan Bossart <[email-masked]> wrote:

> I think we need to tmp++ somewhere here.

Yuk, yes, will fix when caffeinated. Thanks.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 12 Jul 2023 00:43:14 +0200

> On 11 Jul 2023, at 01:26, Daniel Gustafsson <[email-masked]> wrote:

> Yuk, yes, will fix when caffeinated. Thanks.

I did have coffee before now, but only found time to actually address this now
so here is a v7 with just that change and a fresh rebase.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 11 Jul 2023 16:36:06 -0700

> so here is a v7 with just that change and a fresh rebase.

Thanks.  I think the patch is in decent shape.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 31 Aug 2023 23:34:53 +0200

> On 12 Jul 2023, at 01:36, Nathan Bossart <[email-masked]> wrote:
> 

Due to ENOTENOUGHTIME it bitrotted a bit, so here is a v8 rebase which I really
hope to close in this CF.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Wed, 13 Sep 2023 16:12:02 +0200

> hope to close in this CF.

The alignment of this output looks a bit funny:

...
Checking for prepared transactions                            ok
Checking for contrib/isn with bigint-passing mismatch         ok
Checking for data type usage                                  checking all databases
ok
Checking for presence of required libraries                   ok
Checking database user is the install user                    ok
...

Also, you should put gettext_noop() calls into the .status = "Checking ..."
assignments and arrange to call gettext() where they are used, to maintain
the translatability.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 14 Sep 2023 10:48:45 +0200

> On 13 Sep 2023, at 16:12, Peter Eisentraut <[email-masked]> wrote:

> Checking database user is the install user                    ok

I was using the progress reporting to indicate that it hadn't stalled for slow
systems, but it's not probably not all that important really.  Removed such
that "ok" aligns.

> assignments and arrange to call gettext() where they are used, to maintain

Ah, yes of course. Fixed.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 27 Oct 2023 15:20:09 +0200

Attached is a v10 rebase of this patch which had undergone significant bitrot
due to recent changes in the pg_upgrade check phase.  This brings in the
changes into the proposed structure without changes to queries, with no
additional changes to the proposed functionality.

Testing with a completely empty v11 cluster fresh from initdb as the old
cluster shows a significant speedup (averaged over multiple runs, adjusted for
outliers):

patched:  53.59ms (52.78ms, 52.49ms, 55.49ms)
master : 125.87ms (125.23 ms, 125.67ms, 126.67ms)

Using a similarly empty cluster from master as the old cluster shows a smaller
speedup, which is expected since many checks only run for older versions:

patched: 33.36ms (32.82ms, 33.78ms, 33.47ms)
master : 44.87ms (44.73ms, 44.90ms 44.99ms)

The latter case is still pretty interesting IMO since it can speed up testing
where every millisecond gained matters.

--
Daniel Gustafsson

---

From: vignesh C <[email-masked]>
Date: Sat, 27 Jan 2024 09:10:58 +0530

> where every millisecond gained matters.

CFBot shows that the patch does not apply anymore as in [1]:
=== Applying patches on top of PostgreSQL commit ID
55627ba2d334ce98e1f5916354c46472d414bda6 ===
=== applying patch
./v10-0001-pg_upgrade-run-all-data-type-checks-per-connecti.patch
patching file src/bin/pg_upgrade/check.c
Hunk #2 FAILED at 24.
...
1 out of 7 hunks FAILED -- saving rejects to file src/bin/pg_upgrade/check.c.rej

Please post an updated version for the same.

[1] - http://cfbot.cputube.org/patch_46_4200.log

Regards,
Vignesh

---

From: vignesh C <[email-masked]>
Date: Fri, 2 Feb 2024 00:18:25 +0530

> Please post an updated version for the same.

With no update to the thread and the patch still not applying I'm
marking this as returned with feedback.  Please feel free to resubmit
to the next CF when there is a new version of the patch.

Regards,
Vignesh

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 6 Feb 2024 10:32:13 -0600

> to the next CF when there is a new version of the patch.

IMHO this patch is worth trying to get into v17.  I'd be happy to take it
forward if Daniel does not intend to work on it.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 6 Feb 2024 17:47:56 +0100

> On 6 Feb 2024, at 17:32, Nathan Bossart <[email-masked]> wrote:
> 

I actually had the same thought yesterday and spent some time polishing and
rebasing it.  I'll post an updated rebase shortly with the hopes of getting it
committed this week.

--
Daniel Gustafsson

---

From: Nathan Bossart <[email-masked]>
Date: Tue, 6 Feb 2024 10:55:57 -0600

> rebasing it.  I'll post an updated rebase shortly with the hopes of getting it

Oh, awesome.  Thanks!

---

From: Daniel Gustafsson <[email-masked]>
Date: Wed, 7 Feb 2024 14:25:35 +0100

> On 6 Feb 2024, at 17:47, Daniel Gustafsson <[email-masked]> wrote:
> 

Attached is a v11 rebased over HEAD with some very minor tweaks.  Unless there
are objections I plan to go ahead with this version this week.

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Thu, 8 Feb 2024 11:55:19 +0100

> are objections I plan to go ahead with this version this week.

A few more quick comments:

I think the .report_text assignments also need a gettext_noop(), like 
the .status assignments.

The type DataTypesUsageChecks is only used in check.c, so doesn't need 
to be in pg_upgrade.h.

Idea for further improvement: Might be nice if the 
DataTypesUsageVersionCheck struct also included the applicable version 
information, so the additional checks in version.c would no longer be 
necessary.

---

From: Daniel Gustafsson <[email-masked]>
Date: Thu, 8 Feb 2024 15:16:07 +0100

> On 8 Feb 2024, at 11:55, Peter Eisentraut <[email-masked]> wrote:

Thanks for reviewing!

> I think the .report_text assignments also need a gettext_noop(), like the .status assignments.

Done in the attached.

> The type DataTypesUsageChecks is only used in check.c, so doesn't need to be in pg_upgrade.h.

Fixed.

> Idea for further improvement: Might be nice if the DataTypesUsageVersionCheck struct also included the applicable version information, so the additional checks in version.c would no longer be necessary.

I tried various variants of this when writing it, but since the checks aren't
just checking version but also include catalog version checks it became messy.
One option could perhaps be to include a version number for <= comparison, and
if set to zero a function pointer to a version check function must be provided?
That would handle the simple cases in a single place without messy logic, and
leave the more convoluted checks with a special case function.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 9 Feb 2024 00:04:56 +0100

> On 8 Feb 2024, at 15:16, Daniel Gustafsson <[email-masked]> wrote:

> leave the more convoluted checks with a special case function.

The attached is a draft version of this approach, each check can define to run
for all versions, set a threshold version for which it runs or define a
callback which implements a more complicated check.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Fri, 9 Feb 2024 10:33:37 +0100

> On 9 Feb 2024, at 00:04, Daniel Gustafsson <[email-masked]> wrote:
> 

And again pgindented and with documentation on the struct members to make it
easy to add new checks.  A repetitive part of the report text was also moved to
a single place.

--
Daniel Gustafsson

---

From: Daniel Gustafsson <[email-masked]>
Date: Mon, 18 Mar 2024 13:11:41 +0100

Attached is a fresh rebase with only minor cosmetic touch-ups which I would
like to go ahead with during this CF.

Peter: does this address the comments you had on translation and code
duplication?

--
Daniel Gustafsson

---

From: Peter Eisentraut <[email-masked]>
Date: Tue, 19 Mar 2024 08:07:46 +0100

> Peter: does this address the comments you had on translation and code

Yes, this looks good.

---

From: Daniel Gustafsson <[email-masked]>
Date: Tue, 19 Mar 2024 14:38:51 +0100

> On 19 Mar 2024, at 08:07, Peter Eisentraut <[email-masked]> wrote:
> 

Thanks for review! I took another look at this and pushed it.

--
Daniel Gustafsson