PG19 Merge

[fizaaluthra]

Based off of YB master commit 728cbe7cd353a7cb94465a87da8b5ad327a8af74

Resolution notes in pg19mergeresolutions.md

[jasonyb]

This merge is with random pg master commit 90630ec?

[jasonyb]

(I believe this PR does not belong on yugabyte/yugabyte-db as this will never be merged. If you wanted to show the merge for review, it would have better been suited to be on your fork. Unless you intentionally wanted it as a public record to be present in parallel with what actually gets committed.)

[jasonyb]

Review for nodeModifyTable.c:

It is confusing when "parameter" and "argument" are not used to distinguish signature names from passed in expressions. For example, "PG commit 80feb72 added existing as the new oldSlot parameter to ExecUpdate." and "YB commit 161efd6fe7dfa52896dd7e5c0c8641082ab4ef1d and a87fee24675c52723ac4dc9495acf344c165761c changed PG parameters conflictTid, NULL to ybTid, ybOldTuple." both mean arguments. existing is an argument; oldSlot is a parameter.

ExecInitStoredGenerated

• Resolution regarding PG 7fee787 and YB not documented. It seems you accidentally labelled this under ExecComputeStoredGenerated.

ExecInitGenerated

• Same.

ExecUpdateAct

• I don't see anything about if (context->mtstate->operation == CMD_MERGE) PG c649fa2.

Will followup with execIndexing.c review.

[fizaaluthra]

@jasonyb:
I don't see a ExecInitStoredGenerated(just ExecInitGenerated). Addressed everything else I believe.

[OlegLoginov]

Reviewed:

[M] src/bin/pg_dump/pg_backup.h            - [M] - LGTM
[M] src/bin/pg_dump/pg_dump.h              - [M] - LGTM
[M] src/bin/pg_dump/dumputils.h            - [M] - LGTM
[M] src/bin/pg_dump/pg_dump_sort.c         - [M] - LGTM
[M] src/bin/pg_dump/common.c               - [M] - LGTM
[M] src/bin/pg_dump/dumputils.c            - [M] - LGTM
[M] src/bin/pg_dump/pg_backup_archiver.c   - [M] - 3 comments TOFIX,  has YB_TODO_PG19MERGE  
[M] src/bin/pg_dump/pg_dumpall.c             [M] - LGTM,  has YB_TODO_PG19MERGE
[M] src/bin/pg_dump/t/002_pg_dump.pl         [M] - LGTM
[M] src/bin/pg_upgrade/dump.c                [M] - LGTM

Not reviewed yet: src/bin/pg_dump/pg_dump.c

[OlegLoginov]

The line is not needed. (" {")

[OlegLoginov]

The line is not needed. (" }")

[OlegLoginov]

Lost the comment before the line:
memset(&AH->sqlparse, 0, sizeof(AH->sqlparse));

				/*
				 * We've ended on a statement boundary or whitespace (after all,
				 * in upstream Postgres, this sequence of SQL statements is sent
				 * to the backend as one block), so reset the state of sqlparse,
				 * which is normally meant to be stitching together strings with
				 * arbitrary boundaries.
				 */

Please return the comment back.

[fizaaluthra]

good catch, thank you!

[OlegLoginov]

Reviewed: src/bin/pg_dump/pg_dump.c

[OlegLoginov]

Return the comment back (under #if 0 for the moment):

	/*
	 * Binary upgrade mode implies dumping sequence data even in schema-only
	 * mode.  This is not exposed as a separate option, but kept separate
	 * internally for clarity.
	 * YB: Before, during, and after online upgrade, we use the same sequence
	 * data table, so we don't want to write anything to sequence data during
	 * the restore.
	 */

[OlegLoginov]

Try to use original offsets in the block. (To prevent future merge conflicts.)

So, to the PG code we need to add:

 * YB: For backups, follow binary-upgrade mode
 * for inherited child tables to preserve col order.

' +

								  tbinfo->ispartition ||
								  dopt->include_yb_metadata));

[OlegLoginov]

It looks like the block is lost or deleted:

		if (nonemptyReloptions(tbinfo->reloptions) ||
			nonemptyReloptions(tbinfo->toast_reloptions))
		{
			bool		addcomma = false;
          	appendPQExpBufferStr(q, "\nWITH (");
			if (nonemptyReloptions(tbinfo->reloptions))
			{
				addcomma = true;
				appendReloptionsArrayAH(q, tbinfo->reloptions, "", fout);
			}
			if (nonemptyReloptions(tbinfo->toast_reloptions))
			{
				if (addcomma)
					appendPQExpBufferStr(q, ", ");
				appendReloptionsArrayAH(q, tbinfo->toast_reloptions, "toast.",
										fout);
			}
			appendPQExpBufferChar(q, ')');
		}

[OlegLoginov]

Lost the line:

	appendPQExpBuffer(delq, "DROP %s %s;\n", reltypename, qualrelname);

[OlegLoginov]

Actually this change only adds a single space.
It can be done via
appendPQExpBufferStr(q, " ");
Up to you..

[OlegLoginov]

This code block was removed & replaced by YB implementation.

			/*
			 * PRIMARY KEY constraints should not be using NULLS NOT DISTINCT
			 * indexes. Being able to create this was fixed, but we need to
			 * make the index distinct in order to be able to restore the
			 * dump.
			 */
			if (indxinfo->indnullsnotdistinct && coninfo->contype != 'p')
				appendPQExpBufferStr(q, " NULLS NOT DISTINCT");
			appendPQExpBufferStr(q, " (");
			for (k = 0; k < indxinfo->indnkeyattrs; k++)
			{
				appendPQExpBuffer(q, "USING INDEX %s",
								  fmtId(indxinfo->dobj.name));
			}
			if (coninfo->conperiod)
				appendPQExpBufferStr(q, " WITHOUT OVERLAPS");

			if (indxinfo->indnkeyattrs < indxinfo->indnattrs)
				appendPQExpBufferStr(q, ") INCLUDE (");

			for (k = indxinfo->indnkeyattrs; k < indxinfo->indnattrs; k++)  ......

Is it completely covered by the new YB code?
It's hard to compare the old (PG) and the new (YB) logic here.
In YB code I don't see the block:

			for (k = 0; k < indxinfo->indnkeyattrs; k++)
			{
				appendPQExpBuffer(q, "USING INDEX %s",
								  fmtId(indxinfo->dobj.name));

[mtakahar]

Looked at geqo_main.c (wasn't asked though) and createplan.c so far.

[mtakahar]

Revert (take the upstream version.)

This was added by ead09cbeb4e0b84bb5c91c2f44efa5821ac15013, but it was addressed by the upstream commit below. So this has been unnecessary since our upgrade to PG15.

commit dc43fc9b3aa3e0fa9c84faddad6d301813580f88
Author: Tom Lane <tgl@sss.pgh.pa.us>
Date:   2022-01-23 11:09:00 -0500

    Suppress variable-set-but-not-used warning from clang 13.
    
    In the normal configuration where GEQO_DEBUG isn't defined,
    recent clang versions have started to complain that geqo_main.c
    accumulates the edge_failures count but never does anything
    with it.  As a minimal back-patchable fix, insert a void cast
    to silence this warning.  (I'd speculated about ripping out the
    GEQO_DEBUG logic altogether, but I don't think we'd wish to
    back-patch that.)
    
    Per recently-established project policy, this is a candidate
    for back-patching into out-of-support branches: it suppresses
    an annoying compiler warning but changes no behavior.  Hence,
    back-patch all the way to 9.2.
    
    Discussion: https://postgr.es/m/CA+hUKGLTSZQwES8VNPmWO9AO0wSeLt36OCPDAZTccT1h7Q7kTQ@mail.gmail.com

#if defined(ERX)
#if defined(GEQO_DEBUG)
	if (edge_failures != 0)
		elog(LOG, "[GEQO] failures: %d, average: %d",
			 edge_failures, (int) number_generations / edge_failures);
	else
		elog(LOG, "[GEQO] no edge failures detected");
#else
	/* suppress variable-set-but-not-used warnings from some compilers */
	(void) edge_failures;
#endif
#endif

[mtakahar]

Maybe better adding a comment like: /* YB: exposed for use in createplan.c */?

[mtakahar]

Maybe better adding a comment like: /* YB: exposed for use in createplan.c */ here, too?

[mtakahar]

These places looking at enable_xxx boolean flag & adding disable_cost must be updated with path->disabled_nodes setting in all yb_cost_xxx functions.

yugabyte/yugabyte-db#32361

[mtakahar]

Maybe add something like this?

/*
 * YB: exposed for use in costsize.c and selfncs.c:
 * static double get_loop_count(PlannerInfo *root, Index cur_relid, Relids outer_relids);
 */

[mtakahar]

May be add something like this?
/* YB: exposed for use in costsize.c and selfncs.c */

[mtakahar]

These lines are unrelated to skip_lower_saop. Move those out of the #if 0 ... block.
cc @gauravk-in @bmckennaah

I think we can also remove the #if 0 ... since we are not setting it for YB indexes anyways.

[mtakahar]

I think we can just remove this since skip_lower_saop would never be set for YB indexes.

[mtakahar]

This was a poor documentation on excluding index path with distinct pushed down.
It turns out the check was incomplete. @gauravk-in fixed it as well as clarifying in a YB comment. https://phorge.dev.yugabyte.com/D54623

See https://yugabyte.slack.com/archives/CAR5BCH29/p1781630352994689?thread_ts=1781276535.128879&cid=CAR5BCH29.

[mtakahar]

think we can just remove these lines since skip_lower_saop would never be set for YB indexes.

[jasonyb]

Review for execIndexing.c: I skipped "YbExecUpdateIndexTuples indisvalid check" and leave that for @karthik-ramanathan-3006 to review. Everything else LGTM.

Review for execMain.c:

• InitPlan: ++ queryDesc->yb_query_stats = InstrAlloc(queryDesc->instrument_options); was not explained
• ExecPartitionCheckEmitError: based on your ExecBuildSlotValueDescription signature change to align closer to PG's relid, you should do the same in this function: revert the usage of rel and use root_relid instead. Also, this area was not covered in resolution notes.
• ExecConstraints: I found a bug on master and will get back to you
• ExecWithCheckOptions: again, no explanation for the resolution. Incorrectly resolved: empty line was incorrectly added by YB, so it should be removed.