gpdb icon indicating copy to clipboard operation
gpdb copied to clipboard

Set sync/unsync flag for gucs

Open QingMa-gp opened this issue 3 years ago • 5 comments

The GUC's name must be populated into sync_guc_name.h if it needs to sync value between QD and QEs. QD will dispatch its current synced GUC values(as startup options) to create QEs. Otherwise, the settings will not take effect on the newly created QE.

An example of GUC inconsistency between QD and QE:

CREATE OR REPLACE FUNCTION cleanupAllGangs() RETURNS BOOL
AS '@abs_builddir@/../regress/regress.so', 'cleanupAllGangs' LANGUAGE C;

CREATE OR REPLACE FUNCTION public.segment_setting(guc text) RETURNS SETOF text EXECUTE ON ALL SEGMENTS AS
$$ BEGIN RETURN NEXT pg_catalog.current_setting(guc); END $$ LANGUAGE plpgsql;

postgres=# show allow_segment_DML;
 allow_segment_DML
-------------------
 off
(1 row)

postgres=# set allow_segment_DML = on;
SET
postgres=# show allow_segment_DML;
 allow_segment_DML
-------------------
 on
(1 row)

postgres=# select public.segment_setting('allow_segment_DML');
 segment_setting
-----------------
 on
 on
 on
(3 rows)

postgres=# select cleanupAllGangs();
 cleanupallgangs
-----------------
 t
(1 row)

postgres=# show allow_segment_DML;
 allow_segment_DML
-------------------
 on
(1 row)

postgres=# select public.segment_setting('allow_segment_DML');
 segment_setting
-----------------
 off
 off
 off
(3 rows)

Here are some reminders before you submit the pull request

  • [ ] Add tests for the change
  • [ ] Document changes
  • [ ] Communicate in the mailing list if needed
  • [ ] Pass make installcheck
  • [ ] Review a PR in return to support the community

QingMa-gp avatar Aug 17 '22 07:08 QingMa-gp

Side notes while reviewing this PR (which should be tackled as separate PR)

  • GUC gp_enable_slow_writer_testmode should be replaced by fault injector mechanism to test
  • GUC gp_instrument_shmem_size still relevant?
  • GUC gp_safefswritesize, what exactly is the purpose of this? Do we really need it
  • max_appendonly_tables no more a GUC delete the same from file - we really need some mechanism to find stale entries (similar to missing entries) in these sync/unsync files
  • GUC gp_heap_require_relhasoids_match and test_AppendOnlyHash_eviction_vs_just_marking_not_inuse are unused in code and hence should be deleted from guc_gp.c

ashwinstar avatar Aug 18 '22 23:08 ashwinstar

Thanks @QingMa-gp for starting this effort on GUCs, really important GPDB7 readiness task. Once this PR is wrapped up, next we should look into flags assigned for each and every GUC specially the ones inherited from PostgreSQL and make sure it aligns in GPDB (like if GUC is not relevant shouldn't be exposed). If exposed then should be working fine in GPDB providing stated functionally.

ashwinstar avatar Aug 18 '22 23:08 ashwinstar

Sorry @QingMa-gp I found an old issue: https://github.com/greenplum-db/gpdb/issues/6703

Not sure if my previous comment is correct.

kainwen avatar Aug 19 '22 14:08 kainwen

Sorry @QingMa-gp I found an old issue: #6703

Not sure if my previous comment is correct.

Thanks @kainwen . I did some tests based on the master branch. allow_system_table_mods should synchronize to all QEs.

postgres=# SHOW allow_system_table_mods;
 allow_system_table_mods 
-------------------------
 on
(1 row)

postgres=# select public.segment_setting('allow_system_table_mods');
 segment_setting 
-----------------
 off
 off
 off
(3 rows)

postgres=# alter table pg_toast.pg_toast_16389 RENAME TO pg_toast_renamed;
ERROR:  permission denied: "pg_toast_16389" is a system catalog  (seg0 10.117.190.237:7002 pid=30458)

QingMa-gp avatar Aug 22 '22 02:08 QingMa-gp

all these Debug_appendonly_* GUCs need to be in sync file

Done.

QingMa-gp avatar Aug 22 '22 03:08 QingMa-gp