From e6d5232ba2fa50460d800f4e4e72ee1182b64d5b Mon Sep 17 00:00:00 2001 From: Huansong Fu Date: Fri, 26 Aug 2022 15:19:07 -0700 Subject: [PATCH 01/13] Resolve fixme comment in ATExecAddColumn() about why no lock child table In ATExecAddColumn() where we find child tables to apply a flag, we used to open the tables w/o lock. That is because we have already held a lock on the parent and we implicitly believed that no one else could alter the child tables now. That is still correct, but the upstream had introduced an expectation for relation_open() in 4b21acf522d751ba5b6679df391d5121b6c4a35f that all callers of relation_open() should have acquired a lock except in bootstrap mode. I do not think it's important to still use NoLock and try to workaround that assert in relation_open() in some way. Therefore, resolving the FIXME as it is. --- src/backend/commands/tablecmds.c | 17 ++++++----------- 1 file changed, 6 insertions(+), 11 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index fffb0c35a20..822da63c30d 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -8668,17 +8668,12 @@ ATExecAddColumn(List **wqueue, AlteredTableInfo *tab, Relation rel, * We have acquired lockmode on the root and first-level partitions * already. This leaves the deeper subpartitions unlocked, but no * operations can drop (or alter) those relations without locking - * through the root. Note that find_all_inheritors() also includes - * the root partition in the returned list. - * - * GPDB_12_MERGE_FIXME: we used to have NoLock here, but that caused - * assertion failures in the regression tests: - * - * FATAL: Unexpected internal error (relation.c:74) - * DETAIL: FailedAssertion("!(lockmode != 0 || (Mode == BootstrapProcessing) || CheckRelationLockedByMe(r, 1, 1))", File: "relation.c", Line: 74) - * - * so use AccessShareLock instead. Was it important that we used - * NoLock here? + * through the root. But we still lock them to meet the upstream + * expecation in relation_open that all callers should have acquired + * a lock on the table except in bootstrap mode. + + * Note that find_all_inheritors() also includes the root partition + * in the returned list. */ List *all_inheritors = find_all_inheritors(tab->relid, AccessShareLock, NULL); ListCell *lc; From 1bbf201d4f6fdab7a01a85b295364ef935b90f7c Mon Sep 17 00:00:00 2001 From: Annpurna Shahani <30636132+Annu149@users.noreply.github.com> Date: Mon, 29 Aug 2022 11:36:10 +0530 Subject: [PATCH 02/13] gpinitsystem is not working with debug option (#13942) Issue: gpinitsystem is not working with debug option -D and throwing errors as below: 20220715:11:44:09:048391 gpinitsystem:-[INFO]:-End Function PING_HOST /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1051: [: 2>/dev/null: integer expression expected /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1051: [: REMOTE_EXECUTE_AND_GET_OUTPUT: integer expression expected /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1055: [: too many arguments /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1060: [: too many arguments /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1066: [: too many arguments /usr/local/gpdb6/bin/lib/gp_bash_functions.sh: line 1079: [: too many arguments 20220715:11:44:09:048391 gpinitsystem:-[INFO]:-End Function GET_PG_PID_ACTIVE Cause: When we run gpinitsystem with debug option, all the "LOG_MSG" statements are logging(echoing) messages to both STDOUT(console) and log file (As per LOG_MSG function definition present in gpdb/bin/lib/gp_bash_functions.sh). Causing "REMOTE_EXECUTE_AND_GET_OUTPUT" function to echo(print) mutliple messages but only one string is expected to be returned from this function, so the error When we run gpinitsystem without debug option, there is no issue because in that case "LOG_MSG" will log messages to log file only. Fix: Save the initial DEBUG_LEVEL and turn off the debugging at start of this function and then reset DEBUG_LEVEL to INITIAL_DEBUG_LEVEL at end of this function, which will cause all the LOG_MSG statements to log messages only in log file, not echo on STDOUT. Added behave scenario for the above changes --- gpMgmt/bin/lib/gp_bash_functions.sh | 5 +++++ gpMgmt/test/behave/mgmt_utils/gpinitsystem.feature | 13 +++++++++++++ 2 files changed, 18 insertions(+) diff --git a/gpMgmt/bin/lib/gp_bash_functions.sh b/gpMgmt/bin/lib/gp_bash_functions.sh index 3b9a9de7b36..9c1580bb52b 100755 --- a/gpMgmt/bin/lib/gp_bash_functions.sh +++ b/gpMgmt/bin/lib/gp_bash_functions.sh @@ -262,6 +262,9 @@ LOG_MSG () { # Limitation: If the token used for separating command output from banner appears in the begining # of the line in command output/banner output, in that case only partial command output will be returned REMOTE_EXECUTE_AND_GET_OUTPUT () { + INITIAL_DEBUG_LEVEL=$DEBUG_LEVEL + DEBUG_LEVEL=0 + LOG_MSG "[INFO]:-Start Function $FUNCNAME" HOST="$1" CMD="echo 'GP_DELIMITER_FOR_IGNORING_BASH_BANNER';$2" @@ -274,6 +277,8 @@ REMOTE_EXECUTE_AND_GET_OUTPUT () { LOG_MSG "[INFO]:-Completed $TRUSTED_SHELL $HOST $CMD" fi LOG_MSG "[INFO]:-End Function $FUNCNAME" + + DEBUG_LEVEL=$INITIAL_DEBUG_LEVEL #Return output echo "$OUTPUT" } diff --git a/gpMgmt/test/behave/mgmt_utils/gpinitsystem.feature b/gpMgmt/test/behave/mgmt_utils/gpinitsystem.feature index 1eb8a65d9c6..3b666cc3e0e 100644 --- a/gpMgmt/test/behave/mgmt_utils/gpinitsystem.feature +++ b/gpMgmt/test/behave/mgmt_utils/gpinitsystem.feature @@ -304,3 +304,16 @@ Feature: gpinitsystem tests And a demo cluster is created using gpinitsystem args " " And gpinitsystem should return a return code of 0 + Scenario: gpinitsystem should create consistent port entry on segments postgresql.conf file + Given the database is not running + When a demo cluster is created using gpinitsystem args " " + And gpinitsystem should return a return code of 0 + Then gpstate should return a return code of 0 + And check segment conf: postgresql.conf + + Scenario: gpinitsystem creates a cluster successfully when run with -D option + Given create demo cluster config + When the user runs command "gpinitsystem -a -c ../gpAux/gpdemo/clusterConfigFile -D" + Then gpinitsystem should return a return code of 0 + And gpinitsystem should not print "Start Function REMOTE_EXECUTE_AND_GET_OUTPUT" to stdout + And gpinitsystem should not print "End Function REMOTE_EXECUTE_AND_GET_OUTPUT" to stdout From 5a8ae0df21dac11bb2d78e1433814d052a3f39a7 Mon Sep 17 00:00:00 2001 From: Jimmy Yih Date: Thu, 25 Aug 2022 13:08:28 -0700 Subject: [PATCH 03/13] Update gp_switch_wal() to include pg_walfile_name() output The LSN output from pg_switch_wal() is commonly used with pg_walfile_name(). However, the LSN set outputted from gp_switch_wal() cannot be used by pg_walfile_name() because of the very likely timeline differences of the coordinator segment and all the segments. To make sure users/developers that want the WAL segment filename and 100% guarantee that it is correct, we should bake it into gp_switch_wal(). Having a separate catalog function would create a window where HA failover would make the timeline ids incorrect and we would have the same problem all over again. We also convert the function into a C function to guarantee that all the function calls in gp_switch_wal() are called on the correct segments. Using the EXECUTE ON SEGMENT/COORDINATOR combo with a UNION was observed to have known issues when dealing with segment-specific values (specifically the coordinator) and redistribute motion plans. Issue example: ``` postgres=# SELECT gp_segment_id, last_archived_wal FROM gp_stat_archiver ORDER BY gp_segment_id; gp_segment_id | last_archived_wal ---------------+-------------------------- -1 | 000000070000000300000033 0 | 000000060000000200000039 1 | 00000006000000020000003B 2 | 00000006000000020000003D (4 rows) postgres=# create table testtable(a int); CREATE TABLE -- the walfilenames all have the coordinator's timeline id in them postgres=# SELECT gp_segment_id, pg_switch_wal, pg_walfile_name(pg_switch_wal) AS walfilename FROM gp_switch_wal() ORDER BY gp_segment_id; gp_segment_id | pg_switch_wal | walfilename ---------------+---------------+-------------------------- -1 | 3/D0042DB0 | 000000070000000300000034 0 | 2/E8042230 | 00000007000000020000003A 1 | 2/F0042230 | 00000007000000020000003C 2 | 2/F8042230 | 00000007000000020000003E (4 rows) -- these are the expected WAL segment filenames with correct timeline id postgres=# SELECT gp_segment_id, last_archived_wal FROM gp_stat_archiver ORDER BY gp_segment_id; gp_segment_id | last_archived_wal ---------------+-------------------------- -1 | 000000070000000300000034 0 | 00000006000000020000003A 1 | 00000006000000020000003C 2 | 00000006000000020000003E (4 rows) ``` --- src/backend/access/transam/xlogfuncs_gp.c | 129 ++++++++++++++++++ src/backend/catalog/cdb_schema.sql | 31 ----- src/include/catalog/catversion.h | 2 +- src/include/catalog/pg_proc.dat | 4 + .../gpdb_pitr/expected/gpdb_pitr_setup.out | 26 ++-- .../gpdb_pitr/expected/test_gp_switch_wal.out | 81 +++++++++++ src/test/gpdb_pitr/sql/gpdb_pitr_setup.sql | 30 ++-- src/test/gpdb_pitr/sql/test_gp_switch_wal.sql | 43 ++++++ src/test/gpdb_pitr/test_gpdb_pitr.sh | 9 +- 9 files changed, 300 insertions(+), 55 deletions(-) create mode 100644 src/test/gpdb_pitr/expected/test_gp_switch_wal.out create mode 100644 src/test/gpdb_pitr/sql/test_gp_switch_wal.sql diff --git a/src/backend/access/transam/xlogfuncs_gp.c b/src/backend/access/transam/xlogfuncs_gp.c index 145f89e82db..8a7f8ac3935 100644 --- a/src/backend/access/transam/xlogfuncs_gp.c +++ b/src/backend/access/transam/xlogfuncs_gp.c @@ -163,3 +163,132 @@ gp_create_restore_point(PG_FUNCTION_ARGS) SRF_RETURN_DONE(funcctx); } + +/* + * gp_switch_wal: switch WAL on all segments and return meaningful info + */ +Datum +gp_switch_wal(PG_FUNCTION_ARGS) +{ + typedef struct Context + { + CdbPgResults cdb_pgresults; + Datum qd_switch_lsn; + Datum qd_switch_walfilename; + int index; + } Context; + + FuncCallContext *funcctx; + Context *context; + + if (SRF_IS_FIRSTCALL()) + { + MemoryContext oldcontext; + TupleDesc tupdesc; + char *switch_command; + + /* create a function context for cross-call persistence */ + funcctx = SRF_FIRSTCALL_INIT(); + + /* switch to memory context for appropriate multiple function call */ + oldcontext = MemoryContextSwitchTo(funcctx->multi_call_memory_ctx); + + /* create tupdesc for result */ + tupdesc = CreateTemplateTupleDesc(3); + TupleDescInitEntry(tupdesc, (AttrNumber) 1, "segment_id", + INT2OID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 2, "switch_lsn", + LSNOID, -1, 0); + TupleDescInitEntry(tupdesc, (AttrNumber) 3, "switch_walfilename", + TEXTOID, -1, 0); + + funcctx->tuple_desc = BlessTupleDesc(tupdesc); + + context = (Context *) palloc(sizeof(Context)); + context->cdb_pgresults.pg_results = NULL; + context->cdb_pgresults.numResults = 0; + context->index = 0; + funcctx->user_fctx = (void *) context; + + if (!IS_QUERY_DISPATCHER() || Gp_role != GP_ROLE_DISPATCH) + elog(ERROR, + "cannot use gp_switch_wal() when not in QD mode"); + + switch_command = psprintf("SELECT switch_lsn, pg_walfile_name(switch_lsn) FROM pg_catalog.pg_switch_wal() switch_lsn"); + CdbDispatchCommand(switch_command, + DF_NEED_TWO_PHASE | DF_CANCEL_ON_ERROR, + &context->cdb_pgresults); + context->qd_switch_lsn = DatumGetLSN(DirectFunctionCall1(pg_switch_wal, PointerGetDatum(NULL))); + context->qd_switch_walfilename = DirectFunctionCall1(pg_walfile_name, context->qd_switch_lsn); + + pfree(switch_command); + + funcctx->user_fctx = (void *) context; + MemoryContextSwitchTo(oldcontext); + } + + /* + * Using SRF to return all the segment LSN information of the form + * {segment_id, switch_lsn, switch_walfilename} + */ + funcctx = SRF_PERCALL_SETUP(); + context = (Context *) funcctx->user_fctx; + + while (context->index <= context->cdb_pgresults.numResults) + { + Datum values[3]; + bool nulls[3]; + HeapTuple tuple; + Datum result; + Datum switch_lsn; + Datum switch_walfilename; + int seg_index; + + if (context->index == 0) + { + /* Setting fields representing QD's switch WAL */ + seg_index = GpIdentity.segindex; + switch_lsn = context->qd_switch_lsn; + switch_walfilename = context->qd_switch_walfilename; + } + else + { + struct pg_result *pgresult; + ExecStatusType resultStatus; + uint32 hi, lo; + + /* Setting fields representing QE's switch WAL */ + seg_index = context->index - 1; + pgresult = context->cdb_pgresults.pg_results[seg_index]; + resultStatus = PQresultStatus(pgresult); + + if (resultStatus != PGRES_COMMAND_OK && resultStatus != PGRES_TUPLES_OK) + ereport(ERROR, + (errcode(ERRCODE_GP_INTERCONNECTION_ERROR), + (errmsg("could not switch wal from segment"), + errdetail("%s", PQresultErrorMessage(pgresult))))); + Assert(PQntuples(pgresult) == 1); + + sscanf(PQgetvalue(pgresult, 0, 0), "%X/%X", &hi, &lo); + switch_lsn = LSNGetDatum(((uint64) hi) << 32 | lo); + switch_walfilename = CStringGetTextDatum(PQgetvalue(pgresult, 0, 1)); + } + + /* + * Form tuple with appropriate data. + */ + MemSet(values, 0, sizeof(values)); + MemSet(nulls, false, sizeof(nulls)); + + values[0] = Int16GetDatum(seg_index); + values[1] = switch_lsn; + values[2] = switch_walfilename; + tuple = heap_form_tuple(funcctx->tuple_desc, values, nulls); + result = HeapTupleGetDatum(tuple); + + context->index++; + SRF_RETURN_NEXT(funcctx, result); + } + + SRF_RETURN_DONE(funcctx); +} diff --git a/src/backend/catalog/cdb_schema.sql b/src/backend/catalog/cdb_schema.sql index 0423b7733be..36a9342c901 100644 --- a/src/backend/catalog/cdb_schema.sql +++ b/src/backend/catalog/cdb_schema.sql @@ -73,35 +73,4 @@ AS SELECT pg_catalog.gp_execution_segment() as gp_segment_id, * FROM pg_catalog.pg_tablespace_location($1)' LANGUAGE SQL EXECUTE ON COORDINATOR; --- pg_switch_wal wrapper functions to switch WAL segment files on Greenplum cluster-wide -CREATE FUNCTION gp_switch_wal_on_all_segments (OUT gp_segment_id int, OUT pg_switch_wal pg_lsn) -RETURNS SETOF RECORD AS -$$ -DECLARE - seg_id int; -BEGIN - EXECUTE 'select pg_catalog.gp_execution_segment()' INTO seg_id; - -- check if execute in entrydb QE to prevent giving wrong results - IF seg_id = -1 THEN - RAISE EXCEPTION 'Cannot execute in entrydb, this query is not currently supported by GPDB.'; - END IF; - RETURN QUERY SELECT pg_catalog.gp_execution_segment() as gp_segment_id, * FROM pg_catalog.pg_switch_wal(); -END; -$$ LANGUAGE plpgsql EXECUTE ON ALL SEGMENTS; - -CREATE FUNCTION gp_switch_wal (OUT gp_segment_id int, OUT pg_switch_wal pg_lsn) -RETURNS SETOF RECORD -AS - 'SELECT * FROM pg_catalog.gp_switch_wal_on_all_segments() - UNION ALL - SELECT pg_catalog.gp_execution_segment() as gp_segment_id, * FROM pg_catalog.pg_switch_wal()' -LANGUAGE SQL EXECUTE ON COORDINATOR; - -COMMENT ON FUNCTION pg_catalog.gp_switch_wal_on_all_segments() IS 'Switch WAL segment files on all primary segments'; -COMMENT ON FUNCTION pg_catalog.gp_switch_wal() IS 'Switch WAL segment files on all segments'; - -REVOKE EXECUTE ON FUNCTION gp_switch_wal_on_all_segments() FROM public; -REVOKE EXECUTE ON FUNCTION gp_switch_wal() FROM public; - - RESET log_min_messages; diff --git a/src/include/catalog/catversion.h b/src/include/catalog/catversion.h index a66b25fbef0..857724d1153 100644 --- a/src/include/catalog/catversion.h +++ b/src/include/catalog/catversion.h @@ -56,6 +56,6 @@ */ /* 3yyymmddN */ -#define CATALOG_VERSION_NO 302412062 +#define CATALOG_VERSION_NO 302412242 #endif diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat index ac15273e86e..33676ef7665 100644 --- a/src/include/catalog/pg_proc.dat +++ b/src/include/catalog/pg_proc.dat @@ -12512,6 +12512,10 @@ { oid => 6998, descr => 'Create a named restore point on all segments', proname => 'gp_create_restore_point', prorows => '1000', proretset => 't', proparallel => 'u', provolatile => 'v', prorettype => 'record', proargtypes => 'text', prosrc => 'gp_create_restore_point' }, +{ oid => 6996, descr => 'Switch WAL segment files on all segments', + proname => 'gp_switch_wal', prorows => '1000', proretset => 't', proparallel => 'u', provolatile => 'v', prorettype => 'record', proargtypes => '', proexeclocation => 'c', + proallargtypes => '{int2,pg_lsn,text}', proargmodes => '{o,o,o}', proargnames => '{gp_segment_id,pg_switch_wal,pg_walfile_name}', prosrc => 'gp_switch_wal' }, + { oid => 7057, descr => 'compare two complex numbers', proname => 'complex_cmp', prorettype => 'int4', proargtypes => 'complex complex', prosrc => 'complex_cmp' }, diff --git a/src/test/gpdb_pitr/expected/gpdb_pitr_setup.out b/src/test/gpdb_pitr/expected/gpdb_pitr_setup.out index ed90fe4ba38..708b543093b 100644 --- a/src/test/gpdb_pitr/expected/gpdb_pitr_setup.out +++ b/src/test/gpdb_pitr/expected/gpdb_pitr_setup.out @@ -146,24 +146,26 @@ SELECT * FROM gpdb_two_phase_commit_after_restore_point ORDER BY num; 10 (10 rows) -CREATE TEMP TABLE gp_current_wal_lsn AS SELECT -1 AS content_id, pg_current_wal_lsn() AS current_lsn UNION SELECT gp_segment_id AS content_id, pg_current_wal_lsn() FROM gp_dist_random('gp_id'); -CREATE 4 -- Run gp_switch_wal() so that the WAL segment files with the restore --- points are eligible for archival to the WAL Archive directories. -SELECT true FROM gp_switch_wal(); - bool ------- - t - t - t - t -(4 rows) +-- points are eligible for archival to the WAL Archive directories. While +-- we're at it, store the WAL segment filenames that were just archived +-- so that we can check that WAL archival was successful or not later. We +-- must do this in a plpgsql cursor because of a known limitation with +-- CTAS on an EXECUTE ON COORDINATOR function. +CREATE TEMP TABLE switch_walfile_names(content_id smallint, walfilename text); +CREATE +CREATE OR REPLACE FUNCTION populate_switch_walfile_names() RETURNS void AS $$ DECLARE curs CURSOR FOR SELECT * FROM gp_switch_wal(); /*in func*/ DECLARE rec record; /*in func*/ BEGIN /*in func*/ OPEN curs; /*in func*/ LOOP FETCH curs INTO rec; /*in func*/ EXIT WHEN NOT FOUND; /*in func*/ +INSERT INTO switch_walfile_names VALUES (rec.gp_segment_id, rec.pg_walfile_name); /*in func*/ END LOOP; /*in func*/ END $$ LANGUAGE plpgsql; /*in func*/ SELECT populate_switch_walfile_names(); + populate_switch_walfile_names +------------------------------- + +(1 row) -- Ensure that the last WAL segment file for each GP segment was archived. -- This function loops until the archival is complete. It times out after -- approximately 10mins. -CREATE OR REPLACE FUNCTION check_archival() RETURNS BOOLEAN AS $$ DECLARE archived BOOLEAN; /*in func*/ DECLARE archived_count INTEGER; /*in func*/ BEGIN /*in func*/ FOR i in 1..3000 LOOP SELECT bool_and(seg_archived), count(*) FROM (SELECT last_archived_wal = pg_walfile_name(current_lsn) AS seg_archived FROM gp_current_wal_lsn l INNER JOIN gp_stat_archiver a ON l.content_id = a.gp_segment_id) s INTO archived, archived_count; /*in func*/ IF archived AND archived_count = 4 THEN RETURN archived; /*in func*/ END IF; /*in func*/ PERFORM pg_sleep(0.2); /*in func*/ END LOOP; /*in func*/ END $$ LANGUAGE plpgsql; +CREATE OR REPLACE FUNCTION check_archival() RETURNS BOOLEAN AS $$ DECLARE archived BOOLEAN; /*in func*/ DECLARE archived_count INTEGER; /*in func*/ BEGIN /*in func*/ FOR i in 1..3000 LOOP SELECT bool_and(seg_archived), count(*) FROM (SELECT last_archived_wal = l.walfilename AS seg_archived FROM switch_walfile_names l INNER JOIN gp_stat_archiver a ON l.content_id = a.gp_segment_id) s INTO archived, archived_count; /*in func*/ IF archived AND archived_count = 4 THEN RETURN archived; /*in func*/ END IF; /*in func*/ PERFORM pg_sleep(0.2); /*in func*/ END LOOP; /*in func*/ END $$ LANGUAGE plpgsql; CREATE SELECT check_archival(); diff --git a/src/test/gpdb_pitr/expected/test_gp_switch_wal.out b/src/test/gpdb_pitr/expected/test_gp_switch_wal.out new file mode 100644 index 00000000000..be3c387d851 --- /dev/null +++ b/src/test/gpdb_pitr/expected/test_gp_switch_wal.out @@ -0,0 +1,81 @@ +-- Test that gp_switch_wal() returns back WAL segment filenames +-- constructed on the individual segments so that their timeline ids are +-- used instead of each result having the same timeline id. + +-- start_matchsubs +-- +-- # remove line number and entrydb in error message +-- m/\(xlogfuncs_gp\.c\:\d+.*/ +-- s/\(xlogfuncs_gp\.c:\d+.*/\(xlogfuncs_gp\.c:LINE_NUM\)/ +-- +-- end_matchsubs + +-- timeline ids prior to failover/failback should all be 1 due to the +-- test requirement of having a fresh gpdemo cluster with mirrors +SELECT gp_segment_id, substring(pg_walfile_name, 1, 8) FROM gp_switch_wal() ORDER BY gp_segment_id; + gp_segment_id | substring +---------------+----------- + -1 | 00000001 + 0 | 00000001 + 1 | 00000001 + 2 | 00000001 +(4 rows) + +-- stop a primary in order to trigger a mirror promotion +SELECT pg_ctl((SELECT datadir FROM gp_segment_configuration WHERE role = 'p' AND content = 1), 'stop'); + pg_ctl +-------- + OK +(1 row) + +-- trigger failover +select gp_request_fts_probe_scan(); + gp_request_fts_probe_scan +--------------------------- + t +(1 row) + +-- wait for content 1 (earlier mirror, now primary) to finish the promotion +0U: SELECT 1; + ?column? +---------- + 1 +(1 row) + +-- recover the failed primary as new mirror +!\retcode gprecoverseg -a --no-progress; +(exited with code 0) + +-- loop while segments come in sync +SELECT wait_until_all_segments_synchronized(); + wait_until_all_segments_synchronized +-------------------------------------- + OK +(1 row) + +-- rebalance back +!\retcode gprecoverseg -ar --no-progress; +(exited with code 0) + +-- loop while segments come in sync +SELECT wait_until_all_segments_synchronized(); + wait_until_all_segments_synchronized +-------------------------------------- + OK +(1 row) + +-- test that gp_switch_wal() uses the segment-specific timeline id to construct each WAL filename +SELECT gp_segment_id, substring(pg_walfile_name, 1, 8) FROM gp_switch_wal() ORDER BY gp_segment_id; + gp_segment_id | substring +---------------+----------- + -1 | 00000001 + 0 | 00000001 + 1 | 00000003 + 2 | 00000001 +(4 rows) + +-- test simple gp_switch_wal() error scenarios +SELECT gp_switch_wal() FROM gp_dist_random('gp_id'); +ERROR: function with EXECUTE ON restrictions cannot be used in the SELECT list of a query with FROM +CREATE TABLE this_ctas_should_fail AS SELECT gp_segment_id AS contentid, pg_switch_wal, pg_walfile_name FROM gp_switch_wal(); +ERROR: cannot use gp_switch_wal() when not in QD mode (xlogfuncs_gp.c:LINE_NUM) diff --git a/src/test/gpdb_pitr/sql/gpdb_pitr_setup.sql b/src/test/gpdb_pitr/sql/gpdb_pitr_setup.sql index d2de30ba22a..c5b6d9d8fcc 100644 --- a/src/test/gpdb_pitr/sql/gpdb_pitr_setup.sql +++ b/src/test/gpdb_pitr/sql/gpdb_pitr_setup.sql @@ -60,14 +60,28 @@ SELECT * FROM gpdb_two_phase_commit_after_acquire_share_lock; SELECT * FROM gpdb_one_phase_commit; SELECT * FROM gpdb_two_phase_commit_after_restore_point ORDER BY num; -CREATE TEMP TABLE gp_current_wal_lsn AS - SELECT -1 AS content_id, pg_current_wal_lsn() AS current_lsn - UNION - SELECT gp_segment_id AS content_id, pg_current_wal_lsn() FROM gp_dist_random('gp_id'); -- Run gp_switch_wal() so that the WAL segment files with the restore --- points are eligible for archival to the WAL Archive directories. -SELECT true FROM gp_switch_wal(); +-- points are eligible for archival to the WAL Archive directories. While +-- we're at it, store the WAL segment filenames that were just archived +-- so that we can check that WAL archival was successful or not later. We +-- must do this in a plpgsql cursor because of a known limitation with +-- CTAS on an EXECUTE ON COORDINATOR function. +CREATE TEMP TABLE switch_walfile_names(content_id smallint, walfilename text); +CREATE OR REPLACE FUNCTION populate_switch_walfile_names() RETURNS void AS $$ +DECLARE curs CURSOR FOR SELECT * FROM gp_switch_wal(); /*in func*/ +DECLARE rec record; /*in func*/ +BEGIN /*in func*/ + OPEN curs; /*in func*/ + LOOP + FETCH curs INTO rec; /*in func*/ + EXIT WHEN NOT FOUND; /*in func*/ + + INSERT INTO switch_walfile_names VALUES (rec.gp_segment_id, rec.pg_walfile_name); /*in func*/ + END LOOP; /*in func*/ +END $$ +LANGUAGE plpgsql; /*in func*/ +SELECT populate_switch_walfile_names(); -- Ensure that the last WAL segment file for each GP segment was archived. -- This function loops until the archival is complete. It times out after @@ -80,8 +94,8 @@ BEGIN /*in func*/ SELECT bool_and(seg_archived), count(*) FROM (SELECT last_archived_wal = - pg_walfile_name(current_lsn) AS seg_archived - FROM gp_current_wal_lsn l + l.walfilename AS seg_archived + FROM switch_walfile_names l INNER JOIN gp_stat_archiver a ON l.content_id = a.gp_segment_id) s INTO archived, archived_count; /*in func*/ diff --git a/src/test/gpdb_pitr/sql/test_gp_switch_wal.sql b/src/test/gpdb_pitr/sql/test_gp_switch_wal.sql new file mode 100644 index 00000000000..05905328f56 --- /dev/null +++ b/src/test/gpdb_pitr/sql/test_gp_switch_wal.sql @@ -0,0 +1,43 @@ +-- Test that gp_switch_wal() returns back WAL segment filenames +-- constructed on the individual segments so that their timeline ids are +-- used instead of each result having the same timeline id. + +-- start_matchsubs +-- +-- # remove line number and entrydb in error message +-- m/\(xlogfuncs_gp\.c\:\d+.*/ +-- s/\(xlogfuncs_gp\.c:\d+.*/\(xlogfuncs_gp\.c:LINE_NUM\)/ +-- +-- end_matchsubs + +-- timeline ids prior to failover/failback should all be 1 due to the +-- test requirement of having a fresh gpdemo cluster with mirrors +SELECT gp_segment_id, substring(pg_walfile_name, 1, 8) FROM gp_switch_wal() ORDER BY gp_segment_id; + +-- stop a primary in order to trigger a mirror promotion +SELECT pg_ctl((SELECT datadir FROM gp_segment_configuration WHERE role = 'p' AND content = 1), 'stop'); + +-- trigger failover +select gp_request_fts_probe_scan(); + +-- wait for content 1 (earlier mirror, now primary) to finish the promotion +0U: SELECT 1; + +-- recover the failed primary as new mirror +!\retcode gprecoverseg -a --no-progress; + +-- loop while segments come in sync +SELECT wait_until_all_segments_synchronized(); + +-- rebalance back +!\retcode gprecoverseg -ar --no-progress; + +-- loop while segments come in sync +SELECT wait_until_all_segments_synchronized(); + +-- test that gp_switch_wal() uses the segment-specific timeline id to construct each WAL filename +SELECT gp_segment_id, substring(pg_walfile_name, 1, 8) FROM gp_switch_wal() ORDER BY gp_segment_id; + +-- test simple gp_switch_wal() error scenarios +SELECT gp_switch_wal() FROM gp_dist_random('gp_id'); +CREATE TABLE this_ctas_should_fail AS SELECT gp_segment_id AS contentid, pg_switch_wal, pg_walfile_name FROM gp_switch_wal(); diff --git a/src/test/gpdb_pitr/test_gpdb_pitr.sh b/src/test/gpdb_pitr/test_gpdb_pitr.sh index d95dc23c1ea..954a1bf0f4a 100755 --- a/src/test/gpdb_pitr/test_gpdb_pitr.sh +++ b/src/test/gpdb_pitr/test_gpdb_pitr.sh @@ -68,6 +68,12 @@ run_test_isolation2() # Remove temporary test directory if it already exists. [ -d $TEMP_DIR ] && rm -rf $TEMP_DIR +# Create our test database. +createdb gpdb_pitr_database + +# Test output of gp_switch_wal() +run_test_isolation2 test_gp_switch_wal + # Set up WAL Archiving by updating the postgresql.conf files of the # master and primary segments. Afterwards, restart the cluster to load # the new settings. @@ -91,9 +97,6 @@ for segment_role in MASTER PRIMARY1 PRIMARY2 PRIMARY3; do pg_basebackup -h localhost -p ${!PORT_VAR} -X stream -D ${!REPLICA_VAR} --target-gp-dbid ${!REPLICA_DBID_VAR} done -# Create our test database. -createdb gpdb_pitr_database - # Run setup test. This will create the tables, create the restore # points, and demonstrate the commit blocking. run_test_isolation2 gpdb_pitr_setup From 6adbd7c7f7a010ea27aee85c27b023138b9e3880 Mon Sep 17 00:00:00 2001 From: wenru yan Date: Wed, 17 Aug 2022 08:58:22 +0000 Subject: [PATCH 04/13] Disallow Insert and Delete triggers on SplitUpdate Considering that the original command is UPDATE for a SplitUpdate, fire INSERT and DELETE triggers may lead to the wrong action to be enforced. And the triggers in GPDB may require cross segments data changes, disallow the INSERT and DELETE triggers on a SplitUpdate. --- src/backend/executor/nodeModifyTable.c | 25 +++++++++---------------- 1 file changed, 9 insertions(+), 16 deletions(-) diff --git a/src/backend/executor/nodeModifyTable.c b/src/backend/executor/nodeModifyTable.c index 2fcab31e275..20d6988f9c4 100644 --- a/src/backend/executor/nodeModifyTable.c +++ b/src/backend/executor/nodeModifyTable.c @@ -671,11 +671,11 @@ ExecInsert(ModifyTableState *mtstate, * violations before firing these triggers, because they can change the * values to insert. Also, they can run arbitrary user-defined code with * side-effects that we can't cancel by just not inserting the tuple. - */ - /* - * GPDB_12_MERGE_FIXME: PostgreSQL *does* fire INSERT and DELETE - * triggers on an UPDATE that moves tuples from one partition to another. - * Should we follow that example with cross-segment UPDATEs too? + * + * Considering that the original command is UPDATE for a SplitUpdate, fire + * insert triggers may lead to the wrong action to be enforced. And the + * triggers in GPDB may require cross segments data changes, disallow the + * INSERT triggers on a SplitUpdate. */ if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_insert_before_row && @@ -1059,11 +1059,8 @@ ExecInsert(ModifyTableState *mtstate, /* AFTER ROW INSERT Triggers */ /* - * GPDB: Don't fire DELETE triggers on a split UPDATE. - * - * GPDB_12_MERGE_FIXME: PostgreSQL *does* fire INSERT and DELETE - * triggers on an UPDATE that moves tuples from one partition to another. - * Should we follow that example with cross-segment UPDATEs too? + * GPDB: Disallow INSERT triggers on a split UPDATE. See comments in + * BEFORE ROW INSERT Triggers. */ if (!splitUpdate) ExecARInsertTriggers(estate, resultRelInfo, slot, recheckIndexes, @@ -1217,9 +1214,7 @@ ExecDelete(ModifyTableState *mtstate, /* BEFORE ROW DELETE Triggers */ /* - * GPDB_12_MERGE_FIXME: PostgreSQL *does* fire INSERT and DELETE - * triggers on an UPDATE that moves tuples from one partition to another. - * Should we follow that example with cross-segment UPDATEs too? + * Disallow DELETE triggers on a split UPDATE. See comments in ExecInsert(). */ if (resultRelInfo->ri_TrigDesc && resultRelInfo->ri_TrigDesc->trig_delete_before_row && @@ -1511,9 +1506,7 @@ ldelete:; /* AFTER ROW DELETE Triggers */ /* - * GPDB_12_MERGE_FIXME: PostgreSQL *does* fire INSERT and DELETE - * triggers on an UPDATE that moves tuples from one partition to another. - * Should we follow that example with cross-segment UPDATEs too? + * Disallow DELETE triggers on a split UPDATE. See comments in ExecInsert(). */ if (!RelationIsNonblockRelation(resultRelationDesc) && !splitUpdate) { From 8d51a5ee36efe52b09f359afe2d8e3caccc8369e Mon Sep 17 00:00:00 2001 From: Huansong Fu Date: Tue, 30 Aug 2022 11:59:00 -0700 Subject: [PATCH 05/13] Cleanup a FIXME in ATExecAddIndex We used to always dispatch in DefineIndex() but now we do not if it is part of the ALTER TABLE command execution. So we no longer need to skip DefineIndex for QEs. Confirmed and remove this FIXME. --- src/backend/commands/tablecmds.c | 7 ------- 1 file changed, 7 deletions(-) diff --git a/src/backend/commands/tablecmds.c b/src/backend/commands/tablecmds.c index 822da63c30d..36caa4753b8 100644 --- a/src/backend/commands/tablecmds.c +++ b/src/backend/commands/tablecmds.c @@ -10371,13 +10371,6 @@ ATExecAddIndex(AlteredTableInfo *tab, Relation rel, /* The IndexStmt has already been through transformIndexStmt */ Assert(stmt->transformed); - /* The index should already be built if we are a QE */ - /* GPDB_12_MERGE_FIXME: it doesn't seem to work that way anymore. */ -#if 0 - if (Gp_role == GP_ROLE_EXECUTE) - return InvalidObjectAddress; -#endif - /* suppress schema rights check when rebuilding existing index */ check_rights = !is_rebuild; /* skip index build if phase 3 will do it or we're reusing an old one */ From c99d2d853c9346fbbc670bb9079206131e7a3d17 Mon Sep 17 00:00:00 2001 From: David Kimura Date: Thu, 1 Sep 2022 23:01:53 +0000 Subject: [PATCH 06/13] Remove -emit-llvm bitcode compile flag work around This issue does not seeem relevant any more on recent versions of clang. I was able to build with configure flag --with-llvm without any issue on clang 10.0.0. --- gpcontrib/gpcloud/Makefile | 9 --------- src/backend/gpopt/gpopt.mk | 9 --------- src/backend/gporca/gporca.mk | 9 --------- 3 files changed, 27 deletions(-) diff --git a/gpcontrib/gpcloud/Makefile b/gpcontrib/gpcloud/Makefile index 796ab15b4a6..fff38e7c567 100644 --- a/gpcontrib/gpcloud/Makefile +++ b/gpcontrib/gpcloud/Makefile @@ -12,15 +12,6 @@ ifeq ($(DEBUG_S3_SYMBOL),y) PG_CPPFLAGS += -g endif -# GPDB_12_MERGE_FIXME: I had to add these to make "make install" work -# with --with--lvm. I don't undertand what's going on, but I found this -# cure at https://github.com/rdkit/rdkit/issues/2192. Seems to be the -# same issue, whatever it is. -COMPILE.cxx.bc = $(CLANG) -xc++ -Wno-ignored-attributes $(BITCODE_CXXFLAGS) $(CPPFLAGS) -emit-llvm -c -%.bc : %.cpp - $(COMPILE.cxx.bc) -o $@ $< - $(LLVM_BINPATH)/opt -module-summary -f $@ -o $@ - # Targets MODULE_big = gpcloud OBJS = src/gpcloud.o lib/http_parser.o lib/ini.o $(addprefix src/,$(COMMON_OBJS)) diff --git a/src/backend/gpopt/gpopt.mk b/src/backend/gpopt/gpopt.mk index 82ba2e91f5c..76a45e41920 100644 --- a/src/backend/gpopt/gpopt.mk +++ b/src/backend/gpopt/gpopt.mk @@ -1,12 +1,3 @@ -# GPDB_12_MERGE_FIXME: I had to add these to make "make install" work -# with --with--lvm. I don't undertand what's going on, but I found this -# cure at https://github.com/rdkit/rdkit/issues/2192. Seems to be the -# same issue, whatever it is. -COMPILE.cxx.bc = $(CLANG) -xc++ -Wno-ignored-attributes $(BITCODE_CXXFLAGS) $(CPPFLAGS) -emit-llvm -c -%.bc : %.cpp - $(COMPILE.cxx.bc) -o $@ $< - $(LLVM_BINPATH)/opt -module-summary -f $@ -o $@ - override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libgpos/include $(CPPFLAGS) override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libgpopt/include $(CPPFLAGS) override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libnaucrates/include $(CPPFLAGS) diff --git a/src/backend/gporca/gporca.mk b/src/backend/gporca/gporca.mk index 3eca07822bf..78a6c788331 100644 --- a/src/backend/gporca/gporca.mk +++ b/src/backend/gporca/gporca.mk @@ -1,12 +1,3 @@ -# GPDB_12_MERGE_FIXME: I had to add these to make "make install" work -# with --with--lvm. I don't undertand what's going on, but I found this -# cure at https://github.com/rdkit/rdkit/issues/2192. Seems to be the -# same issue, whatever it is. -COMPILE.cxx.bc = $(CLANG) -xc++ -Wno-ignored-attributes $(BITCODE_CXXFLAGS) $(CPPFLAGS) -emit-llvm -c -%.bc : %.cpp - $(COMPILE.cxx.bc) -o $@ $< - $(LLVM_BINPATH)/opt -module-summary -f $@ -o $@ - override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libgpos/include $(CPPFLAGS) override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libgpopt/include $(CPPFLAGS) override CPPFLAGS := -I$(top_srcdir)/src/backend/gporca/libnaucrates/include $(CPPFLAGS) From 1fadf71dc91200df4ff06bddfcbd495e3e524b1a Mon Sep 17 00:00:00 2001 From: Rakesh Sharma Date: Fri, 2 Sep 2022 13:27:47 +0530 Subject: [PATCH 07/13] change verify checksum FIXME to FEATURE NOT SUPPORTED --- gpMgmt/bin/gppylib/commands/pg.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/gpMgmt/bin/gppylib/commands/pg.py b/gpMgmt/bin/gppylib/commands/pg.py index f630477ed50..d147c42c182 100644 --- a/gpMgmt/bin/gppylib/commands/pg.py +++ b/gpMgmt/bin/gppylib/commands/pg.py @@ -222,7 +222,7 @@ def __init__(self, target_datadir, source_host, source_port, create_slot=False, cmd_tokens.extend(self._xlog_arguments(replication_slot_name)) - # GPDB_12_MERGE_FIXME: avoid checking checksum for heap tables + # GPDB_12_MERGE_FEATURE_NOT_SUPPORTED: avoid checking checksum for heap tables # till we code logic to skip/verify checksum for # appendoptimized tables. Enabling this results in basebackup # failures with appendoptimized tables. From 52b54cbb8ea01f692bce44fbe80ed5390bd6dc1c Mon Sep 17 00:00:00 2001 From: Jimmy Yih Date: Mon, 29 Aug 2022 17:44:13 -0700 Subject: [PATCH 08/13] Fix db_size_functions regress test The test started failing after a change to `gp_switch_wal()` was made. The db_size_functions regress test was not updated accordingly. Fix the test by simply updating the expected output (matchsub in the test added as well). GPDB commit reference: https://github.com/greenplum-db/gpdb/commit/d3163467b018082b1b36d70f94fe95e59898b6e0 --- src/test/regress/expected/db_size_functions.out | 11 ++++++++--- src/test/regress/sql/db_size_functions.sql | 7 +++++++ 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/src/test/regress/expected/db_size_functions.out b/src/test/regress/expected/db_size_functions.out index e45b84e5ea1..2445ba4be5f 100644 --- a/src/test/regress/expected/db_size_functions.out +++ b/src/test/regress/expected/db_size_functions.out @@ -6,6 +6,13 @@ -- to collect the totals across segments, and to support AO / AOCS tables. -- Hence, we better have extra tests for those things. -- +-- start_matchsubs +-- +-- # remove line number and entrydb in error message +-- m/\(xlogfuncs_gp\.c\:\d+.*/ +-- s/\(xlogfuncs_gp\.c:\d+.*/\(xlogfuncs_gp\.c:LINE_NUM\)/ +-- +-- end_matchsubs -- The total depends on the number of segments, and will also change whenever -- the built-in objects change, so be lenient. -- As of this writing, the total size of template0 database, across three segments, @@ -118,9 +125,7 @@ ERROR: Cannot execute in entrydb, this query is not currently supported by GPDB CONTEXT: PL/pgSQL function gp_tablespace_segment_location(oid) line 8 at RAISE SQL function "gp_tablespace_location" statement 1 create temp table t1 as select gp_segment_id as seg_id from gp_switch_wal(); -ERROR: Cannot execute in entrydb, this query is not currently supported by GPDB. -CONTEXT: PL/pgSQL function gp_switch_wal_on_all_segments() line 8 at RAISE -SQL function "gp_switch_wal" statement 1 +ERROR: cannot use gp_switch_wal() when not in QD mode (xlogfuncs_gp.c:LINE_NUM) -- -- Tests on the table and index size variants. -- diff --git a/src/test/regress/sql/db_size_functions.sql b/src/test/regress/sql/db_size_functions.sql index 63ef0a5f3d0..67f7ec7bb7e 100644 --- a/src/test/regress/sql/db_size_functions.sql +++ b/src/test/regress/sql/db_size_functions.sql @@ -7,6 +7,13 @@ -- Hence, we better have extra tests for those things. -- +-- start_matchsubs +-- +-- # remove line number and entrydb in error message +-- m/\(xlogfuncs_gp\.c\:\d+.*/ +-- s/\(xlogfuncs_gp\.c:\d+.*/\(xlogfuncs_gp\.c:LINE_NUM\)/ +-- +-- end_matchsubs -- The total depends on the number of segments, and will also change whenever -- the built-in objects change, so be lenient. From a964b90c0da96c2393882762ed588bc998f7d006 Mon Sep 17 00:00:00 2001 From: "Huiliang.liu" Date: Tue, 6 Sep 2022 14:34:56 +0800 Subject: [PATCH 09/13] Fix a panic case in the greenplum_fdw test. (#14033) * Fix a panic case in the postgres_fdw test. It happens in the below query: select foo.f1, loct1.f1 from foo join loct1 on (foo.f1 = loct1.f1) order by foo.f2 offset 10 limit 10; The DDL statements are: ``` CREATE EXTENSION greenplum_fdw; CREATE SERVER loopback FOREIGN DATA WRAPPER greenplum_fdw OPTIONS (host 'localhost', port '15432', dbname 'gpadmin'); CREATE USER MAPPING FOR CURRENT_USER SERVER loopback; create table foo (f1 int, f2 int); create table loct1 (f1 int, f2 int, f3 int); create foreign table foo2 (f3 int) inherits (foo) server loopback options (table_name 'loct1', use_remote_estimate 'true'); ``` The reason is that set_append_path_locus()->cdbpath_create_motion_path() returns NULL (i.e. can not create a motion to reach the target locus requirement) but set_append_path_locus() does not handle this bad case and panic due to this (i.e. when accessing subpath->sameslice_relids while subpath is NULL). This happens because in the above query, foo has a child with foreign table (and a local heap table child additionally) and the append node tries to create a parameterized path (one is an index scan path and another is the foreign scan path; the target locus is Entry). This happens when use_remote_estimate is set on the foreign table. Per postgresGetForeignPaths() code only when this option is set, parameterized path for foreign scan is considered. Fixing this by returning NULL path for the case that set_append_path_locus() returns false. Callers of them should take care of the NULL returning cases. Co-authored-by: Huiliang.liu Co-authored-by: Paul Guo --- src/backend/optimizer/util/pathnode.c | 18 +++++++++++++----- 1 file changed, 13 insertions(+), 5 deletions(-) diff --git a/src/backend/optimizer/util/pathnode.c b/src/backend/optimizer/util/pathnode.c index 80231107403..5b6572e1e3b 100644 --- a/src/backend/optimizer/util/pathnode.c +++ b/src/backend/optimizer/util/pathnode.c @@ -77,7 +77,7 @@ static List *reparameterize_pathlist_by_child(PlannerInfo *root, List *pathlist, RelOptInfo *child_rel); -static void set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, +static bool set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, List *pathkeys, int parallel_workers, bool parallel_aware); static CdbPathLocus adjust_modifytable_subpath(PlannerInfo *root, CmdType operation, @@ -1476,7 +1476,8 @@ create_append_path(PlannerInfo *root, else pathnode->limit_tuples = -1.0; - set_append_path_locus(root, (Path *)pathnode, rel, NIL, parallel_workers, parallel_aware); + if (!set_append_path_locus(root, (Path *)pathnode, rel, NIL, parallel_workers, parallel_aware)) + return NULL; foreach(l, pathnode->subpaths) { @@ -1611,7 +1612,8 @@ create_merge_append_path(PlannerInfo *root, * Add Motions to the child nodes as needed, and determine the locus * of the MergeAppend itself. */ - set_append_path_locus(root, (Path *) pathnode, rel, pathkeys, 0, false); + if (!set_append_path_locus(root, (Path *) pathnode, rel, pathkeys, 0, false)) + return NULL; /* * Add up the sizes and costs of the input paths. @@ -1679,8 +1681,9 @@ create_merge_append_path(PlannerInfo *root, * Set the locus of an Append or MergeAppend path. * * This modifies the 'subpaths', costs fields, and locus of 'pathnode'. + * It will return false if fails to create motion for parameterized path. */ -static void +static bool set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, List *pathkeys, int parallel_workers, bool parallel_aware) { @@ -1712,7 +1715,7 @@ set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, if (!subpaths) { CdbPathLocus_MakeGeneral(&pathnode->locus); - return; + return true; } /* @@ -2110,6 +2113,9 @@ set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, subpath = cdbpath_create_motion_path(root, subpath, pathkeys, false, targetlocus); else subpath = cdbpath_create_motion_path(root, subpath, NIL, false, targetlocus); + + if (subpath == NULL) + return false; } pathnode->sameslice_relids = bms_union(pathnode->sameslice_relids, subpath->sameslice_relids); @@ -2159,6 +2165,8 @@ set_append_path_locus(PlannerInfo *root, Path *pathnode, RelOptInfo *rel, pathnode->parallel_workers = targetlocus.parallel_workers; *subpaths_out = new_subpaths; + + return true; } /* From daa71cc59d3351f906e5eaad7e7ed091ca0c0bb8 Mon Sep 17 00:00:00 2001 From: Xing Guo Date: Wed, 7 Sep 2022 08:41:05 +0800 Subject: [PATCH 10/13] [psql] Add support for describing auxiliary tables for ao table. (#14063) This patch adds support for describing auxiliary tables. e.g., ``` postgres=# create table foo(i int) with (appendonly=true); postgres=# create index on foo(i); CREATE INDEX postgres=# select blkdirrelid::regclass from pg_appendonly where relid = 'foo'::regclass; blkdirrelid ---------------------------- pg_aoseg.pg_aoblkdir_16411 (1 row) postgres=# \d+ pg_aoseg.pg_aoblkdir_16411 Appendonly block directory table: "pg_aoseg.pg_aoblkdir_16411" Column | Type | Storage ----------------+---------+--------- segno | integer | plain columngroup_no | integer | plain first_row_no | bigint | plain minipage | bytea | plain Indexes: "pg_aoblkdir_16411_index" PRIMARY KEY, btree (segno, columngroup_no, first_row_no) postgres=# \d+ pg_aoseg.pg_aovisimap_16411 Appendonly visibility map table: "pg_aoseg.pg_aovisimap_16411" Column | Type | Storage --------------+---------+--------- segno | integer | plain first_row_no | bigint | plain visimap | bytea | plain Indexes: "pg_aovisimap_16411_index" PRIMARY KEY, btree (segno, first_row_no) postgres=# \d+ pg_aoseg.pg_aoseg_16411 Appendonly segment entry table: "pg_aoseg.pg_aoseg_16411" Column | Type | Storage -----------------+----------+--------- segno | integer | plain eof | bigint | plain tupcount | bigint | plain varblockcount | bigint | plain eofuncompressed | bigint | plain modcount | bigint | plain formatversion | smallint | plain state | smallint | plain ``` --- src/bin/psql/describe.c | 17 ++++++++++++++++- 1 file changed, 16 insertions(+), 1 deletion(-) diff --git a/src/bin/psql/describe.c b/src/bin/psql/describe.c index ca48d71f6d4..e5eac604f83 100644 --- a/src/bin/psql/describe.c +++ b/src/bin/psql/describe.c @@ -2542,6 +2542,18 @@ describeOneTableDetails(const char *schemaname, printfPQExpBuffer(&title, _("Partitioned table \"%s.%s\""), schemaname, relationname); break; + case RELKIND_AOSEGMENTS: + printfPQExpBuffer(&title, _("Appendonly segment entry table: \"%s.%s\""), + schemaname, relationname); + break; + case RELKIND_AOVISIMAP: + printfPQExpBuffer(&title, _("Appendonly visibility map table: \"%s.%s\""), + schemaname, relationname); + break; + case RELKIND_AOBLOCKDIR: + printfPQExpBuffer(&title, _("Appendonly block directory table: \"%s.%s\""), + schemaname, relationname); + break; default: /* untranslated unknown relkind */ printfPQExpBuffer(&title, "?%c? \"%s.%s\"", @@ -2948,7 +2960,10 @@ describeOneTableDetails(const char *schemaname, tableinfo.relkind == RELKIND_FOREIGN_TABLE || tableinfo.relkind == RELKIND_PARTITIONED_TABLE || tableinfo.relkind == RELKIND_PARTITIONED_INDEX || - tableinfo.relkind == RELKIND_TOASTVALUE) + tableinfo.relkind == RELKIND_TOASTVALUE || + tableinfo.relkind == RELKIND_AOSEGMENTS || + tableinfo.relkind == RELKIND_AOBLOCKDIR || + tableinfo.relkind == RELKIND_AOVISIMAP) { /* Footer information about a table */ PGresult *result = NULL; From d15799bdc189778133353229a4d679425b33ce17 Mon Sep 17 00:00:00 2001 From: Wenru Yan <94830465+yanwr1@users.noreply.github.com> Date: Thu, 8 Sep 2022 20:18:35 +0800 Subject: [PATCH 11/13] Fix wrong results with a WITH RECURSIVE query (#13715) In postgres, multiple recursive slef-references is disallowed by the SQL spec, and prevent inlining of multiply-referenced CTEs with outer recursive refs, so they only have one WorkTable correlate to one RecursiveUnion. But in GPDB, we don't support CTE scan plan, if a WITH RECURSIVE query has multiply-referenced CTEs with outer recursive, there will be multiple WorkTable scans correlate to one RecursiveUnion, and they share the same readptr of working table, so that it will lose some datas. Create a readptr for each WorkTable scan so that they won't influence each other. In postgres, it can generate CTE SubPlans for WITH subqueries, if a CTE subplan have outer recursive refs with outer recursive CTE, it will exist a WorkTable in subplan. And initialize state information for subplans will be before initialize on the main query tree, so there are corner cases where we'll get the init call before the RecursiveUnion does. IN GPDB, we don't have CTE scan node, so we won't generate CTE subplan for WITH subqueries, also we won't call the ExecInitWorkTableScan func before ExecInitRecursiveUnion. Set rustate in the INIT step. since shareinputscan with outer refs is not supported by GPDB, if contain outer self references, the cte also need to be inlined. Remove GPDB_12_MERGE_FIXME in subselect.sql which describes the issue. --- src/backend/executor/nodeRecursiveunion.c | 6 ++ src/backend/executor/nodeWorktablescan.c | 69 +++++++++---------- src/backend/optimizer/path/allpaths.c | 8 +++ src/backend/optimizer/plan/subselect.c | 7 +- src/include/nodes/execnodes.h | 13 ++++ src/include/optimizer/subselect.h | 1 + src/test/regress/expected/subselect.out | 41 +++++------ .../regress/expected/subselect_optimizer.out | 32 ++++++--- src/test/regress/sql/subselect.sql | 6 -- 9 files changed, 104 insertions(+), 79 deletions(-) diff --git a/src/backend/executor/nodeRecursiveunion.c b/src/backend/executor/nodeRecursiveunion.c index caa30d3b5d7..f2f33b395df 100644 --- a/src/backend/executor/nodeRecursiveunion.c +++ b/src/backend/executor/nodeRecursiveunion.c @@ -128,6 +128,11 @@ ExecRecursiveUnion(PlanState *pstate) /* intermediate table becomes working table */ node->working_table = node->intermediate_table; + for (int k = 1; k < node->refcount; k++) + { + /* The work table hasn't been scanned yet, it must be at start, don't need to rescan here */ + tuplestore_alloc_read_pointer(node->working_table, EXEC_FLAG_REWIND); + } /* create new empty intermediate table */ node->intermediate_table = tuplestore_begin_heap(false, false, @@ -199,6 +204,7 @@ ExecInitRecursiveUnion(RecursiveUnion *node, EState *estate, int eflags) rustate->intermediate_empty = true; rustate->working_table = tuplestore_begin_heap(false, false, work_mem); rustate->intermediate_table = tuplestore_begin_heap(false, false, work_mem); + rustate->refcount = 0; /* * If hashing, we need a per-tuple memory context for comparisons, and a diff --git a/src/backend/executor/nodeWorktablescan.c b/src/backend/executor/nodeWorktablescan.c index f4394e9f244..c179e213836 100644 --- a/src/backend/executor/nodeWorktablescan.c +++ b/src/backend/executor/nodeWorktablescan.c @@ -55,6 +55,7 @@ WorkTableScanNext(WorkTableScanState *node) Assert(ScanDirectionIsForward(node->ss.ps.state->es_direction)); tuplestorestate = node->rustate->working_table; + tuplestore_select_read_pointer(tuplestorestate, node->readptr); /* * Get the next tuple from tuplestore. Return NULL if no more tuples. @@ -87,40 +88,6 @@ ExecWorkTableScan(PlanState *pstate) { WorkTableScanState *node = castNode(WorkTableScanState, pstate); - /* - * On the first call, find the ancestor RecursiveUnion's state via the - * Param slot reserved for it. (We can't do this during node init because - * there are corner cases where we'll get the init call before the - * RecursiveUnion does.) - */ - if (node->rustate == NULL) - { - WorkTableScan *plan = (WorkTableScan *) node->ss.ps.plan; - EState *estate = node->ss.ps.state; - ParamExecData *param; - - param = &(estate->es_param_exec_vals[plan->wtParam]); - Assert(param->execPlan == NULL); - Assert(!param->isnull); - node->rustate = castNode(RecursiveUnionState, DatumGetPointer(param->value)); - Assert(node->rustate); - - /* - * The scan tuple type (ie, the rowtype we expect to find in the work - * table) is the same as the result rowtype of the ancestor - * RecursiveUnion node. Note this depends on the assumption that - * RecursiveUnion doesn't allow projection. - */ - ExecAssignScanType(&node->ss, - ExecGetResultType(&node->rustate->ps)); - - /* - * Now we can initialize the projection info. This must be completed - * before we can call ExecScan(). - */ - ExecAssignScanProjectionInfo(&node->ss); - } - return ExecScan(&node->ss, (ExecScanAccessMtd) WorkTableScanNext, (ExecScanRecheckMtd) WorkTableScanRecheck); @@ -135,6 +102,7 @@ WorkTableScanState * ExecInitWorkTableScan(WorkTableScan *node, EState *estate, int eflags) { WorkTableScanState *scanstate; + ParamExecData *param; /* check for unsupported flags */ /* @@ -156,7 +124,27 @@ ExecInitWorkTableScan(WorkTableScan *node, EState *estate, int eflags) scanstate->ss.ps.plan = (Plan *) node; scanstate->ss.ps.state = estate; scanstate->ss.ps.ExecProcNode = ExecWorkTableScan; - scanstate->rustate = NULL; /* we'll set this later */ + + /* In postgres, it can generate CTE SubPlans for WITH subqueries, if a CTE subplan have outer + * recursive refs with outer recursive CTE, it will exist a WorkTable in subplan. And initialize + * state information for subplans will be before initialize on the main query tree, so there are + * corner cases where we'll get the init call before the RecursiveUnion does. + * IN GPDB, we don't have CTE scan node, so we won't generate CTE subplan for WITH subqueries, + * also we won't call the ExecInitWorkTableScan func before ExecInitRecursiveUnion. Set rustate + * in the INIT step. + */ + param = &(estate->es_param_exec_vals[node->wtParam]); + Assert(param->execPlan == NULL); + Assert(!param->isnull); + scanstate->rustate = castNode(RecursiveUnionState, DatumGetPointer(param->value)); + if (scanstate->rustate->refcount == 0) + scanstate->readptr = 0; + else + { + /* during node init, the work table hasn't been scanned yet, it must be at start, don't need to rescan here*/ + scanstate->readptr = tuplestore_alloc_read_pointer(scanstate->rustate->working_table, EXEC_FLAG_REWIND); + } + scanstate->rustate->refcount++; /* * Miscellaneous initialization @@ -174,7 +162,13 @@ ExecInitWorkTableScan(WorkTableScan *node, EState *estate, int eflags) scanstate->ss.ps.resultopsset = true; scanstate->ss.ps.resultopsfixed = false; - ExecInitScanTupleSlot(estate, &scanstate->ss, NULL, &TTSOpsMinimalTuple); + /* The scan tuple type (ie, the rowtype we expect to find in the work + * table) is the same as the result rowtype of the ancestor + * RecursiveUnion node. Note this depends on the assumption that + * RecursiveUnion doesn't allow projection. + */ + ExecInitScanTupleSlot(estate, &scanstate->ss, ExecGetResultType(&scanstate->rustate->ps), &TTSOpsMinimalTuple); + ExecAssignScanProjectionInfo(&scanstate->ss); /* * initialize child expressions @@ -228,5 +222,8 @@ ExecReScanWorkTableScan(WorkTableScanState *node) /* No need (or way) to rescan if ExecWorkTableScan not called yet */ if (node->rustate) + { + tuplestore_select_read_pointer(node->rustate->working_table, node->readptr); tuplestore_rescan(node->rustate->working_table); + } } diff --git a/src/backend/optimizer/path/allpaths.c b/src/backend/optimizer/path/allpaths.c index a3d0064baea..325409589de 100644 --- a/src/backend/optimizer/path/allpaths.c +++ b/src/backend/optimizer/path/allpaths.c @@ -46,6 +46,7 @@ #include "optimizer/planmain.h" #include "optimizer/planner.h" #include "optimizer/restrictinfo.h" +#include "optimizer/subselect.h" #include "optimizer/tlist.h" #include "optimizer/planshare.h" #include "parser/parse_clause.h" @@ -2982,6 +2983,13 @@ set_cte_pathlist(PlannerInfo *root, RelOptInfo *rel, RangeTblEntry *rte) } + /* + * since shareinputscan with outer refs is not supported by GPDB, if + * contain outer self references, the cte need to be inlined. + */ + if (is_shared && contain_outer_selfref(cte->ctequery)) + is_shared = false; + if (!is_shared) { PlannerConfig *config = CopyPlannerConfig(root->config); diff --git a/src/backend/optimizer/plan/subselect.c b/src/backend/optimizer/plan/subselect.c index d940e813c66..41ecf8dbb13 100644 --- a/src/backend/optimizer/plan/subselect.c +++ b/src/backend/optimizer/plan/subselect.c @@ -121,6 +121,7 @@ static bool finalize_primnode(Node *node, finalize_primnode_context *context); static bool finalize_agg_primnode(Node *node, finalize_primnode_context *context); extern double global_work_mem(void); +static bool contain_outer_selfref_walker(Node *node, Index *depth); /* * Get the datatype/typmod/collation of the first column of the plan's output. @@ -1342,11 +1343,11 @@ contain_dml_walker(Node *node, void *context) } return expression_tree_walker(node, contain_dml_walker, context); } - +#endif /* * contain_outer_selfref: is there an external recursive self-reference? */ -static bool +bool contain_outer_selfref(Node *node) { Index depth = 0; @@ -1397,7 +1398,7 @@ contain_outer_selfref_walker(Node *node, Index *depth) return expression_tree_walker(node, contain_outer_selfref_walker, (void *) depth); } - +#if 0 /* * inline_cte: convert RTE_CTE references to given CTE into RTE_SUBQUERYs */ diff --git a/src/include/nodes/execnodes.h b/src/include/nodes/execnodes.h index b7d1817b496..a10c120e17f 100644 --- a/src/include/nodes/execnodes.h +++ b/src/include/nodes/execnodes.h @@ -1445,6 +1445,7 @@ typedef struct MergeAppendState * * recursing T when we're done scanning the non-recursive term * intermediate_empty T if intermediate_table is currently empty + * refcount number of WorkTableScans which will scan the working table * working_table working table (to be scanned by recursive term) * intermediate_table current recursive output (next generation of WT) * ---------------- @@ -1454,6 +1455,7 @@ typedef struct RecursiveUnionState PlanState ps; /* its first field is NodeTag */ bool recursing; bool intermediate_empty; + int refcount; Tuplestorestate *working_table; Tuplestorestate *intermediate_table; /* Remaining fields are unused in UNION ALL case */ @@ -2133,11 +2135,22 @@ typedef struct NamedTuplestoreScanState * WorkTableScan nodes are used to scan the work table created by * a RecursiveUnion node. We locate the RecursiveUnion node * during executor startup. + * In postgres, multiple recursive self-references is disallowed + * by the SQL spec, and prevent inlining of multiply-referenced + * CTEs with outer recursive refs, so they only have one WorkTable + * correlate to one RecursiveUnion. But in GPDB, we don't support + * CTE scan plan, if exists multiply-referenced CTEs with outer + * recursive, there will be multiple WorkTable scan correlate to + * one RecursiveUnion, and they share the same readptr of working + * table which cause wrong results. We create a readptr for each + * WorkTable scan, so that they won't influence each other. + * * ---------------- */ typedef struct WorkTableScanState { ScanState ss; /* its first field is NodeTag */ + int readptr; /* index of work table's tuplestore read pointer */ RecursiveUnionState *rustate; } WorkTableScanState; diff --git a/src/include/optimizer/subselect.h b/src/include/optimizer/subselect.h index cc8d0b6bda8..884b8172e96 100644 --- a/src/include/optimizer/subselect.h +++ b/src/include/optimizer/subselect.h @@ -54,5 +54,6 @@ extern void check_multi_subquery_correlated(PlannerInfo *root, Var *var); extern List *generate_subquery_vars(PlannerInfo *root, List *tlist, Index varno); extern bool QueryHasDistributedRelation(Query *q, bool recursive); +extern bool contain_outer_selfref(Node *node); #endif /* SUBSELECT_H */ diff --git a/src/test/regress/expected/subselect.out b/src/test/regress/expected/subselect.out index 2fee5d171d6..427fdf060d4 100644 --- a/src/test/regress/expected/subselect.out +++ b/src/test/regress/expected/subselect.out @@ -1821,11 +1821,6 @@ select * from x, x x2 where x.n = x2.n; (19 rows) -- Multiply-referenced CTEs can't be inlined if they contain outer self-refs --- start_ignore --- GPDB_12_MERGE_FIXME: This currenty produces incorrect results on GPDB. --- It's not a new issue, but it was exposed by this new upstream test case --- with the PostgreSQL v12 merge. --- See https://github.com/greenplum-db/gpdb/issues/10014 explain (verbose, costs off) with recursive x(a) as ((values ('a'), ('b')) @@ -1834,25 +1829,22 @@ with recursive x(a) as select z.a || z1.a as a from z cross join z as z1 where length(z.a || z1.a) < 5)) select * from x; - QUERY PLAN ----------------------------------------------------------- - CTE Scan on x - Output: x.a - CTE x - -> Recursive Union - -> Values Scan on "*VALUES*" - Output: "*VALUES*".column1 - -> Nested Loop - Output: (z.a || z1.a) - Join Filter: (length((z.a || z1.a)) < 5) - CTE z - -> WorkTable Scan on x x_1 - Output: x_1.a - -> CTE Scan on z - Output: z.a - -> CTE Scan on z z1 - Output: z1.a -(16 rows) + QUERY PLAN +--------------------------------------------------- + Recursive Union + -> Values Scan on "*VALUES*" + Output: "*VALUES*".column1 + -> Nested Loop + Output: (x.a || x_1.a) + Join Filter: (length((x.a || x_1.a)) < 5) + -> WorkTable Scan on x + Output: x.a + -> Materialize + Output: x_1.a + -> WorkTable Scan on x x_1 + Output: x_1.a + Optimizer: Postgres query optimizer +(13 rows) with recursive x(a) as ((values ('a'), ('b')) @@ -1887,7 +1879,6 @@ select * from x; bbbb (22 rows) --- end_ignore explain (verbose, costs off) with recursive x(a) as ((values ('a'), ('b')) diff --git a/src/test/regress/expected/subselect_optimizer.out b/src/test/regress/expected/subselect_optimizer.out index 643cca693fc..3855edf1b96 100644 --- a/src/test/regress/expected/subselect_optimizer.out +++ b/src/test/regress/expected/subselect_optimizer.out @@ -1882,11 +1882,6 @@ select * from x, x x2 where x.n = x2.n; (19 rows) -- Multiply-referenced CTEs can't be inlined if they contain outer self-refs --- start_ignore --- GPDB_12_MERGE_FIXME: This currenty produces incorrect results on GPDB. --- It's not a new issue, but it was exposed by this new upstream test case --- with the PostgreSQL v12 merge. --- See https://github.com/greenplum-db/gpdb/issues/10014 explain (verbose, costs off) with recursive x(a) as ((values ('a'), ('b')) @@ -1920,13 +1915,32 @@ with recursive x(a) as select z.a || z1.a as a from z cross join z as z1 where length(z.a || z1.a) < 5)) select * from x; - a ---- + a +------ a b -(2 rows) + aa + ab + ba + bb + aaaa + aaab + aaba + aabb + abaa + abab + abba + abbb + baaa + baab + baba + babb + bbaa + bbab + bbba + bbbb +(22 rows) --- end_ignore explain (verbose, costs off) with recursive x(a) as ((values ('a'), ('b')) diff --git a/src/test/regress/sql/subselect.sql b/src/test/regress/sql/subselect.sql index 308f11baf5c..ec886efe905 100644 --- a/src/test/regress/sql/subselect.sql +++ b/src/test/regress/sql/subselect.sql @@ -925,11 +925,6 @@ with x as not materialized (select * from (select f1, current_database() as n fr select * from x, x x2 where x.n = x2.n; -- Multiply-referenced CTEs can't be inlined if they contain outer self-refs --- start_ignore --- GPDB_12_MERGE_FIXME: This currenty produces incorrect results on GPDB. --- It's not a new issue, but it was exposed by this new upstream test case --- with the PostgreSQL v12 merge. --- See https://github.com/greenplum-db/gpdb/issues/10014 explain (verbose, costs off) with recursive x(a) as ((values ('a'), ('b')) @@ -946,7 +941,6 @@ with recursive x(a) as select z.a || z1.a as a from z cross join z as z1 where length(z.a || z1.a) < 5)) select * from x; --- end_ignore explain (verbose, costs off) with recursive x(a) as From 2652f70287857ae5e7534d60e1661a2d2151093f Mon Sep 17 00:00:00 2001 From: Soumyadeep Chakraborty Date: Wed, 20 Jul 2022 11:00:21 -0700 Subject: [PATCH 12/13] Fix gpcheckcat false alarms for pg_default_acl gpcheckcat currently flags default privilege objects as having missing dependencies. This is because it searches for dependencies of pg_default_acl oids in pg_depend instead of in pg_shdepend. The fix is to recognize pg_default_acl as a table outside the purview of pg_depend. Minimal repro: postgres=# CREATE ROLE foo; postgres=# ALTER DEFAULT PRIVILEGES FOR ROLE foo REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC; postgres=# SELECT oid, * FROM pg_default_acl; oid | defaclrole | defaclnamespace | defaclobjtype | defaclacl -------+------------+-----------------+---------------+------------- 17304 | 17303 | 0 | f | {foo=X/foo} (1 row) postgres=# SELECT * FROM pg_shdepend WHERE objid = 17304; dbid | classid | objid | objsubid | refclassid | refobjid | deptype -------+---------+-------+----------+------------+----------+--------- 12812 | 826 | 17304 | 0 | 1260 | 17303 | o (2 rows) $ gpcheckcat postgres ... Object oid: 17304 Table name: pg_default_acl Name of test which found this issue: dependency_pg_default_acl Table pg_default_acl has a dependency issue on oid 17304 at content -1 Table pg_default_acl has a dependency issue on oid 17304 at content 0 Table pg_default_acl has a dependency issue on oid 17304 at content 1 Table pg_default_acl has a dependency issue on oid 17304 at content 2 --- gpMgmt/bin/gppylib/gpcatalog.py | 1 + gpMgmt/test/behave/mgmt_utils/gpcheckcat.feature | 13 +++++++++++++ 2 files changed, 14 insertions(+) diff --git a/gpMgmt/bin/gppylib/gpcatalog.py b/gpMgmt/bin/gppylib/gpcatalog.py index 2342f492fb6..2b29ee8569f 100644 --- a/gpMgmt/bin/gppylib/gpcatalog.py +++ b/gpMgmt/bin/gppylib/gpcatalog.py @@ -78,6 +78,7 @@ class GPCatalogException(Exception): 'pg_compression', 'pg_conversion', 'pg_database', + 'pg_default_acl', 'pg_enum', 'pg_namespace', 'pg_resgroup', diff --git a/gpMgmt/test/behave/mgmt_utils/gpcheckcat.feature b/gpMgmt/test/behave/mgmt_utils/gpcheckcat.feature index 42ca7cf485e..ff8108362d4 100644 --- a/gpMgmt/test/behave/mgmt_utils/gpcheckcat.feature +++ b/gpMgmt/test/behave/mgmt_utils/gpcheckcat.feature @@ -561,6 +561,19 @@ Feature: gpcheckcat tests Then gpcheckcat should return a return code of 3 And the user runs "dropdb mis_attr_db" + Scenario: gpcheckcat should not report dependency error from pg_default_acl + Given database "check_dependency_error" is dropped and recreated + And the user runs "psql -d check_dependency_error -c "CREATE ROLE foo; ALTER DEFAULT PRIVILEGES FOR ROLE foo REVOKE EXECUTE ON FUNCTIONS FROM PUBLIC;"" + Then psql should return a return code of 0 + When the user runs "gpcheckcat check_dependency_error" + Then gpcheckcat should return a return code of 0 + And gpcheckcat should not print "SUMMARY REPORT: FAILED" to stdout + And gpcheckcat should not print "has a dependency issue on oid" to stdout + And gpcheckcat should print "Found no catalog issue" to stdout + And the user runs "dropdb check_dependency_error" + And the user runs "psql -c "DROP ROLE foo"" + + ########################### @concourse_cluster tests ########################### # The @concourse_cluster tag denotes the scenario that requires a remote cluster From 2f9157367ede0e6f32396e1dce39c5d2574339e1 Mon Sep 17 00:00:00 2001 From: Brent Doil Date: Thu, 8 Sep 2022 15:04:17 -0400 Subject: [PATCH 13/13] pg_upgrade: Fix core dump in report_progress() The report_progress function may take a NULL argument for cluster, but was not checking it before trying to print cluster->major_version_str, resulting in a core dump. Add a check before fprintf call, which also resolves a FIXME. Also use --progress flag in pg_upgrade regression test. Authored-by: Brent Doil --- src/bin/pg_upgrade/greenplum/reporting.c | 14 ++++++++------ src/bin/pg_upgrade/test_gpdb.sh | 2 +- 2 files changed, 9 insertions(+), 7 deletions(-) diff --git a/src/bin/pg_upgrade/greenplum/reporting.c b/src/bin/pg_upgrade/greenplum/reporting.c index bf6ed4fee72..30022f55ebf 100644 --- a/src/bin/pg_upgrade/greenplum/reporting.c +++ b/src/bin/pg_upgrade/greenplum/reporting.c @@ -94,13 +94,15 @@ report_progress(ClusterInfo *cluster, progress_type op, char *fmt,...) } /* - * GPDB_12_MERGE_FIXME: The CLUSTER_NAME macro was removed, so we've - * changed to printing the major version of the cluster instead. This may - * well be good enough (or even better), but some more thought should go - * into this before calling it done. + * In the case where cluster is NULL, omit the cluster version in + * the progress file. */ - fprintf(progress_file, "%lu;%s;%s;%s;\n", - epoch_us(), cluster->major_version_str, opname(op), message); + if (cluster && *cluster->major_version_str != '\0') + fprintf(progress_file, "%lu;%s;%s;%s;\n", + epoch_us(), cluster->major_version_str, opname(op), message); + else + fprintf(progress_file, "%lu;%s;%s;\n", epoch_us(), opname(op), message); + progress_counter++; /* diff --git a/src/bin/pg_upgrade/test_gpdb.sh b/src/bin/pg_upgrade/test_gpdb.sh index 798ac91c635..d8dd53742a9 100755 --- a/src/bin/pg_upgrade/test_gpdb.sh +++ b/src/bin/pg_upgrade/test_gpdb.sh @@ -172,7 +172,7 @@ upgrade_qd() # Run pg_upgrade pushd $1 - time ${NEW_BINDIR}/pg_upgrade --mode=dispatcher --old-bindir=${OLD_BINDIR} --old-datadir=$2 --new-bindir=${NEW_BINDIR} --new-datadir=$3 ${PGUPGRADE_OPTS} + time ${NEW_BINDIR}/pg_upgrade --mode=dispatcher --progress --old-bindir=${OLD_BINDIR} --old-datadir=$2 --new-bindir=${NEW_BINDIR} --new-datadir=$3 ${PGUPGRADE_OPTS} if (( $? )) ; then echo "ERROR: Failure encountered in upgrading qd node" exit 1