- 27 10月, 2020 1 次提交
-
-
由 Heikki Linnakangas 提交于
In commit c5f6dbbe, we changed the row and cost estimates on plan nodes to represent per-segment costs. That made some estimates worse, because the effects of the estimate "clamping" compounds. Per my comment on the PR back then: > One interesting effect of this change, that explains many of the > plan changes: If you have a table with very few rows, or e.g. a qual > like id = 123 that matches exactly one row, the Seq/Index Scan on it > will be marked with rows=1. It now means that we estimate that every > segment returns one row, although in reality, only one of them will > return a row, and the rest will return nothing. That's because the > row count estimates are "clamped" in the planner to at least > 1. That's not a big deal on its own, but if you then have e.g. a > Gather Motion on top of the Scan, the planner will estimate that the > Gather Motion returns as many rows as there are segments. If you > have e.g. 100 segments, that's relatively a big discrepancy, with > 100 rows vs 1. I don't think that's a big problem in practice, I > don't think most plans are very sensitive to that kind of a > misestimate. What do you think? > > If we wanted to fix that, perhaps we should stop "clamping" the > estimates to 1. I don't think there's any fundamental reason we need > to do it. Perhaps clamp down to 1 / numsegments instead. But I came up with a less intrusive idea, implemented in this commit: Most Motion nodes have a "parent" RelOptInfo, and the RelOptInfo contains an estimate of the total number of rows, before dividing it with the number of segments or clamping. So if the row estimate we get from the subpath seems clamped to 1.0, we look at the row estimate on the underlying RelOptInfo instead, and use that if it's smaller. That makes the row count estimates better for plans that fetch a single row or a few rows, same as they were before commit c5f6dbbe. Not all RelOptInfos have a row count estimate, and the subpaths estimate is more accurate if the number of rows produced by the path differs from the number of rows in the underlying relation, e.g. because of a ProjectSet node, so we still prefer the subpath's estimate if it doesn't seem clamped. Reviewed-by: NZhenghua Lyu <zlv@pivotal.io>
-
- 26 10月, 2020 3 次提交
-
-
由 Adam Lee 提交于
Otherwise it will raise an exception "command not run yet".
-
由 Heikki Linnakangas 提交于
It was handled in expression_tree_mutator(), which is why everything worked, but that's not the right place. expression_tree_mutator() is supposed to handle nodes that can appear in expressions, and plan_tree_mutator() is supposed to handle Plan nodes. Reviewed-by: NZhenghua Lyu <zlv@pivotal.io>
-
由 dh-cloud 提交于
In buildGangDefinition, newGangDefinition->db_descriptors are initialized one by one, but newGangDefinition->size was already set to its final value. If an error was caught, its size should be reset to the right number.
-
- 24 10月, 2020 1 次提交
-
-
由 David Krieger 提交于
The tests in commit be5d11e2 contained a typo that caused the changes in the Scenario "gpstart starts even if the standby host is unreachable" to not properly cleanup after itself. Though the test feature still passes, this leaves a bug to be found later when more tests are added.
-
- 23 10月, 2020 17 次提交
-
-
由 Asim R P 提交于
Append-optimized tables do not contain transaction information in their tuples. Therefore, pg_class.relfrozenxid must remain invalid. This is being done correctly during table creation, however, when the table was rewritten, the relfrozenxid was accidentally set. Fix it such that diff with upstream is minimised. In particular, the function "should_have_valid_relfozenxid" is removed. The fixme comments that led me to this bug are also removed. Reviewed by: Ashwin Agrawal
-
由 dh-cloud 提交于
Postgresql libpq document: > Note that when PQconnectStart or PQconnectStartParams returns a > non-null pointer, you must call PQfinish when you are finished > with it, in order to dispose of the structure and any associated > memory blocks. **This must be done even if the connection attempt > fails or is abandoned**. However, cdbconn_disconnect() function did not call PQfinish when CONNECTION_BAD, it can cause socket leaks (CLOSE_WAIT state).
-
由 Adam Lee 提交于
It didn't log the error message before if pg_rewind fails, fix that to make DBA/field/developer's life eaisier. Before this: ``` 20201022:15:19:10:011118 gprecoverseg:earth:adam-[INFO]:-Running pg_rewind on required mirrors 20201022:15:19:12:011118 gprecoverseg:earth:adam-[WARNING]:-Incremental recovery failed for dbid 2. You must use gprecoverseg -F to recover the segment. 20201022:15:19:12:011118 gprecoverseg:earth:adam-[INFO]:-Starting mirrors 20201022:15:19:12:011118 gprecoverseg:earth:adam-[INFO]:-era is 0406b847bf226356_201022151031 ``` After this: ``` 20201022:15:33:31:019577 gprecoverseg:earth:adam-[INFO]:-Running pg_rewind on required mirrors 20201022:15:33:31:019577 gprecoverseg:earth:adam-[WARNING]:-pg_rewind: fatal: could not find common ancestor of the source and target cluster's timelines 20201022:15:33:31:019577 gprecoverseg:earth:adam-[WARNING]:-Incremental recovery failed for dbid 2. You must use gprecoverseg -F to recover the segment. 20201022:15:33:31:019577 gprecoverseg:earth:adam-[INFO]:-Starting mirrors 20201022:15:33:31:019577 gprecoverseg:earth:adam-[INFO]:-era is 0406b847bf226356_201022151031 ```
-
由 David Kimura 提交于
-
由 Jesse Zhang 提交于
This conforms our code to the best practice that each polymorphic function should have exactly one of "virtual", "override", and "final" specifier. I made this change using the following invocation: clang-tidy -checks '-*,modernize-use-override' Once we've made this change, ideally this practice can be enforced in CI. We can just run clang-tidy with a single "modernize-use-override" check to start with. Or we can see if our compilers are helpful enough. Fortunately Clang already issues warnings (turned into errors by -Werror) when we have inconsistent use of "override", and GCC appears to have something similar (-Wsuggest-override) in versions 9.2 and later.
-
由 Jesse Zhang 提交于
Now that we are explicitly declaring copy-assignment operators and copy constructors as deleted, we should also make them public -- private and =delete don't make much sense in combination, and this results in the best diagnostics in practice [1]. In the previous commit where private unimplemented special member functions are changed to be declared as deleted, we left the new declarations private. This follows up by moving those deleted functions to the public section of their classes. I made this change guided by tooling: even though it doesn't offer a fix, clang-tidy is useful enough to emit diagnostics that look like the following: /home/pivotal/workspace/gpdb/src/backend/gporca/libgpopt/include/gpopt/base/CColRef.h:76:2: warning: deleted member function should be public [modernize-use-equals-delete] CColRef(const CColRef &) = delete; ^ The process involved of making this change is a tale of much sorrow, blood and tears. Suffice it to say 20 different regular expressions were involved, and I'm edging closer to a PTSD when looking at backslashes. References: [1] https://abseil.io/tips/143#summary
-
由 Jesse Zhang 提交于
This commit replaces two pre-C++ 11 practices with their modern, more intent-expressive equivalents: "disallowed" copy and assignments, and pairs of empty braces for special member function (destructors, default constructors, and etc) bodies. "Disallowed" copy constructors / assignment ------------------------------------------- Old: private, unimplemented copy constructors and copy-assignment operators in classes. These are usually paired with a semi-descriptive comment. New: publicly declare them as "= delete". Fun fact: we had 13 spellings in comments on disallowed copy constructors: 01. disable copy ctor 02. hidden copy ctor 03. inaccessible copy ctor 04. no copy ctor 05. no default copy ctor 06. private copy ctor 07. private no copy ctor 08. disabled copy constructor 09. no copy constructor 10. private copy constructor 11. copy c'tor - not defined 12. no copy c'tor 13. private copy c'tor This commit removes all of them, because the new code ("~T() = delete") already clearly expresses the intent of disallowing copy and assignment. To keep the history clear, this commit leaves the declaration of "prohibited" functions private. A forthcoming commit will wholesale change them to public. Defaulted special member functions ---------------------------------- Old: struct A { A() {} ~A(); }; A::~A() {} New: struct A { A() = default; ~A(); }; A::~A() = default; Replacing empty braces with defaulting not only makes them more clear, they also enable more opportunities in compiler optimizations, as e.g. some defaulted functions might be recognized as trivial. Most of this commit is produced by running clang-tidy with an invocation like the following (plus some CMake and shell tricks): clang-tidy-12 -header-filter 'gpdbcost|gpopt|gpos|naucrates' -checks '-*,modernize-use-equals-delete,modernize-use-equals-default' The tool uses a slightly conservative heuristic to detect a large portion of the two outdated patterns above and rewrite them into using "= delete" and "= default". Making the "= delete" functions public, is sadly a FIXME item, so we'll have to do it by hand (in a forthcoming commit).
-
由 Jesse Zhang 提交于
The current setup in CMake (and Makefile's too, but that's an even harder problem) leads to equivalent-but-not-identical header include paths (-I) for the same directory, e.g. -Ilibgpopt/include vs -Ilibgpdbcost/../libgpopt/include . This confuses Clang-based tooling into identifying multiple paths for the same header -- the difference being the extra sibling directory followed by dot-dot, or "niece" directory followed by two levels of dot-dot, and so forth -- as different headers. That, in turn, undermines the conflict resolution and edit deduplication features in Clang's refactoring engine, leading to duplicate edits when applying FixIt's. This commit applies some fairly simple fixes to spell the sibling directories in a way that generates consistent include paths, so that Clang tooling are more functional. The Makefile's are left unchanged as that is a lot more difficult to make "right". One can argue that we _might_ want to instead transform the intermediate representation of Clang's "replacement" YAML files, but that's left for another day.
-
由 Jesse Zhang 提交于
During the development of a forthcoming commit that replaces private unimplemented special member functions with explicitly deleted ones, we got a surprising improvement of compiler diagnostics: Clang started to see a lot of unused private fields that are masked by the fact that there were unimplemented functions in the class. This makes sense because the ostensibly unused field _could be_ used by a method whose definition compiler hasn't seen in the current translation unit -- it also suggests that the compiler would be better equipped at detecting this if we had used whole-program analysis. A sample of the errors looks like the following: In file included from CMiniDumper.cpp:14: In file included from ../../../../../../src/backend/gporca/libgpos/include/gpos/error/CErrorContext.h:17: ../../../../../../src/backend/gporca/libgpos/include/gpos/error/CMiniDumper.h:32:15: error: private field 'm_mp' is not used [-Werror,-Wunused-private-field] CMemoryPool *m_mp; ^ 1 error generated. There are 12 such warnings, and this commit fixes them all. Note that code mortality is a chain reaction: oftentimes, a variable (including parameter) is only live because it's passed to another variable. While I was at it, I also performed chained removal of variables and parameters that became dead after removing the dead fields.
-
由 Jesse Zhang 提交于
We have a bunch of classes with private fields that are used only in debug builds, some of them are probably opportunities for complete removal. This is exposed by an upcoming commit that enforces the use of "=delete" and "=default" throughout the codebase. I've attempted to solve this by adding the GPOS_ASSERTS_ONLY attribute, but GCC doesn't like that, throwing errors like the following: In file included from ../src/backend/gporca/libgpos/include/gpos/common/clibwrapper.h:22, from ../src/backend/gporca/libgpos/include/gpos/error/CMessage.h:21, from ../src/backend/gporca/libgpos/include/gpos/error/CMessageTable.h:14, from ../src/backend/gporca/libgpos/include/gpos/error/CMessageRepository.h:14, from ../src/backend/gporca/libgpos/src/_api.cpp:15: ../src/backend/gporca/libgpos/include/gpos/attributes.h:15:43: error: ‘unused’ attribute ignored [-Werror=attributes] 15 | #define GPOS_UNUSED __attribute__((unused)) | ^ ../src/backend/gporca/libgpos/include/gpos/attributes.h:21:27: note: in expansion of macro ‘GPOS_UNUSED’ 21 | #define GPOS_ASSERTS_ONLY GPOS_UNUSED | ^~~~~~~~~~~ ../src/backend/gporca/libgpos/include/gpos/memory/CAutoMemoryPool.h:56:31: note: in expansion of macro ‘GPOS_ASSERTS_ONLY’ 56 | ELeakCheck m_leak_check_type GPOS_ASSERTS_ONLY; | ^~~~~~~~~~~~~~~~~ cc1plus: all warnings being treated as errors So we're back to the good ol' #ifdef GPOS_DEBUG. This is the first of a pair of manual changes. In the immediately following commit I'll remove the remaining (unconditionally) unused private fields.
-
由 Ashwin Agrawal 提交于
Seems resource group run_tests task didn't configure with Python before and seems image doesn't have PYTHON, as changing the configure default failed the job. Hence, explicitely adding --without-python to retain old functionality with changed default.
-
由 David Kimura 提交于
This allows us to reduce code duplication of workload SQL scripts
-
由 David Kimura 提交于
-
由 Ashwin Agrawal 提交于
Reviewed-by: NPaul Guo <guopa@vmware.com>
-
由 Ashwin Agrawal 提交于
Many tests in GPDB depend on plpython, hence enabling it by default is helpful, instead of required to specify everytime. Given python is needed on developer system for GPDB Management Scripts so its not adding any new dependency. Discussion: https://groups.google.com/a/greenplum.org/g/gpdb-dev/c/laqc2YXxCWU/m/fS6ojeDIAQAJReviewed-by: NPaul Guo <guopa@vmware.com>
-
由 Ashwin Agrawal 提交于
gpcloud has dependency on libxml. gpcloud enabled by default and libxml disabled by default in configure, creates a weird pattern where at least one option needs to be specified for configure to pass, just ./configure never passes for Greenplum. ICG and isolation2 tests just pass without it so seems a better route to disable gpcloud. Alternatively, can enable libxml by default but seems conflicts with upstream and seems unnecessary dependency to add for routine development. Discussion: https://groups.google.com/a/greenplum.org/g/gpdb-dev/c/laqc2YXxCWU/m/fS6ojeDIAQAJReviewed-by: NPaul Guo <guopa@vmware.com>
-
由 Shaoqi Bai 提交于
Postgres 991f3e5a introduce new error message fields, but cdbdisp_get_PQerror() did not extract these newly added message field. These fields client cannot see from libpq. This fixes https://github.com/greenplum-db/gpdb/issues/7934. Co-authored-by: NAshwin Agrawal <aashwin@vmware.com> Reviewed-by: NHeikki Linnakangas <hlinnaka@iki.fi>
-
- 22 10月, 2020 3 次提交
-
-
由 Jamie McAtamney 提交于
Currently, if a host is unreachable when gpstart is run, it will not report this and will instead fail with an error that is both inaccurate and unhelpful to the user, such as claiming that checksums are invalid for segments on a given host when it simply can't reach that host to verify the checksums. This commit adds a check to verify that all hosts are reachable before beginning the startup process and, if one or more hosts are not reachable, marks segments on those hosts down (in gparray, not in the cluster) so gpstart won't try to run any checks against unreachable hosts and so that the cluster can still be started in this state so long as there are otherwise enough valid segments to start it.
-
由 Adam Lee 提交于
Commit 4f85fde8 refactored the infrastructure for interrupt processing, simplified signal handlers and it had no more need to perform StartTransactionCommand()...CommitTransactionCommand() call inside any signal handler. So the AmIInSIGUSR1Handler() should not happen at all, remove it.
-
由 Jimmy Yih 提交于
To allow experimentation of various Postgres recovery features, we must handle distributed commit WAL records similarly to commit and commit prepared WAL records. This will allow the standby master segment to utilize the recovery features. The mirror segments already get handled since they use the regular commit and commit prepared WAL records. With this change, the standby master segment will be able to properly output a value for a pg_last_xact_replay_timestamp() call (while in hot standby), have delayed WAL replay, and use PITR/archive recovery settings such as recovery_target_time and recovery_target_xid.
-
- 21 10月, 2020 3 次提交
-
-
由 dh-cloud 提交于
In function DropResourceGroup(), group->lockedForDrop is set to true by calling ResGroupCheckForDrop, however, it can only be set to false inside dropResgroupCallback. This callback is registered at the ending of function DropResourceGroup. If an error occured between them, group->lockedForDrop would be true forever. Fix it by putting the register process ahead of the lock call. To prevent Assert(group->nRunning* > 0) if ResGroupCheckForDrop throws an error, return directly if group->lockedForDrop did not change. See: ``` gpconfig -c gp_resource_manager -v group gpstop -r -a psql # CREATE RESOURCE GROUP resg_test WITH ( CPU_RATE_LIMIT=20, MEMORY_LIMIT=20, CONCURRENCY=50, MEMORY_SHARED_QUOTA=80, MEMORY_SPILL_RATIO=20, MEMORY_AUDITOR=vmtracker ); # CREATE USER user_test PASSWORD '123456' RESOURCE GROUP resg_test; # DROP RESOURCE GROUP resg_test; psql -U user_test > \d -- hang ```
-
由 Kalen Krempely 提交于
Use GPHOME directly. There is no need for GPSEARCH. This enables demo_cluster.sh to handle symlinks which is useful when GPDB is installed using RPMs and a demo cluster is desired.
-
由 Aleksey Kashin 提交于
The parameters were incorrectly passed while gprecoverseg was invoked causing gprecoverseg to fail. Co-authored-by: Bhuvnesh Chaudhary<bchaudhary@vmware.com>
-
- 20 10月, 2020 1 次提交
-
-
由 Valery Khashkovsky 提交于
https://github.com/greenplum-db/gpdb/issues/8558 Upstream's pg_regress supports starting a temporary instance and running tests against it with `--temp-instance`, but Greenplum's could not. This commit is working around it from the make file. Signed-off-by: NAdam Lee <adlee@vmware.com>
-
- 19 10月, 2020 3 次提交
-
-
由 Peifeng Qiu 提交于
Client package test on windows and the related binary was removed by commit c2574cc4 and 8bb057ac, but there's still one usage in rename_rc_artifacts task. Remove it also to fix release candidate job.
-
由 Jinbao Chen 提交于
The result of NULL not in an unempty set is false. The result of NULL not in an empty set is true. But if an unempty set has partitioned locus. This set will be divided into several subsets. Some subsets may be empty. Because NULL not in empty set equals true. There will be some tuples that shouldn't exist in the result set. The patch disable the partitioned locus of inner table by removing the join clause from the redistribution_clauses. Co-authored-by: NHubert Zhang <hubertzhang@apache.org> Co-authored-by: NRichard Guo <riguo@pivotal.io>
-
由 Fang Zheng 提交于
The buildGpQueryString() function should use palloc() instead of palloc0() to allocate the buffer for holding the serialized query string, thus avoiding the unnecessary cost of setting all bytes in buffer to zero. Reviewed-by: NHeikki Linnakangas <hlinnaka@iki.fi> Reviewed-by: NAshwin Agrawal <aashwin@vmware.com>
-
- 17 10月, 2020 2 次提交
-
-
由 Paul Guo 提交于
Test partition1 creates table hhh_r1 also and that test runs with test partition in parallel with schedule file greenplum_schedule, and this could cause below test failure. Fixing this by renaming the table name. @@ -523,9 +523,7 @@ partition aa start (date '2007-01-01') end (date '2008-01-01') every (interval '0 days') ); -ERROR: EVERY parameter too small -LINE 5: every (interval '0 days') - ^ +ERROR: relation "hhh_r1" already exists create table foo_p (i int) distributed by(i) partition by range(i) (start (1) end (20) every(0)); Reviewed-by: NAsim R P <pasim@vmware.com>
-
由 Ning Yu 提交于
Reviewed-by: NAshwin Agrawal <aashwin@vmware.com>
-
- 14 10月, 2020 2 次提交
-
-
由 Ashwin Agrawal 提交于
Reviewed-by: NPaul Guo <pguo@pivotal.io>
-
由 Ashwin Agrawal 提交于
Reviewed-by: NPaul Guo <pguo@pivotal.io>
-
- 13 10月, 2020 4 次提交
-
-
由 Ashwin Agrawal 提交于
plpython3u extension can already exist hence put it under {start_ignore end_ignore} block.
-
由 Ashwin Agrawal 提交于
Given we have simpler mechanism to have all test helpers as part of setup test, no more need to have parsing "include:" syntax in sql isolation framework. One less thing to depend in python.
-
由 Ashwin Agrawal 提交于
Given now setup test runs internally once, having all helper functions in one place with it seems better. Avoids per test overhead to parse and run these helper include files.
-
由 Ashwin Agrawal 提交于
This eliminates need to remember running setup for some tests which depend on it. Plus, all the prerequisite functions can be combined in setup and run once. Irrespective of way tests are triggered like via schedule file or singleton, setup will be run and create the required functions once automatically.
-