Conversation
For context, the registry-image resource was introduced more than two years ago as a drop-in replace for the docker-image resource (especially when you're only fetching, not building). It's leaner, less resource-intense, faster, and doesn't rely on spawning a Docker daemon. Also swept up in this patch are four unused files that are left behind by previous changes. This is spiritually a cherry-pick of commit be6ff30 (greenplum-db/gpdb#10818). See https://github.com/concourse/registry-image-resource for more.
Function `RelationGetIndexList()` does not filter out invalid indexes. That responsiblity is left to the caller (e.g. `get_relation_info()`). Issue is that Orca was not checking index validity. This commit also introduces an optimization to Orca that is already used in Planner whereby we first check relhasindex before checking pg_index. (cherry picked from commit b011c35)
Commit 0cf1c0d defined a global variable in a header. This resulted in a ton of "unused variable" warnings. The `static step_timer timer` is only used in contrib/pg_upgrade/util.c. So move it here.
Co-authored-by: Xin Zhang <zhxin@vmware.com> Co-authored-by: Brent Doli <bdoil@vmware.com>
…se_legacy_hashops (#10810) When gp_use_legacy_hashops is set, for plans that have an Aggregate, ORCA might crash when computing the distribution spec. This occurs when the grouping columns are of a type that have default hash distribution opfamilies, but do not have legacy hash distribution opfamilies. This happens because the distribution is done only for types for which "IsRedistributable()" returns true. This is (incorrectly) determined by looking (only) at the default opfamily (independent of the GUC). Subsequently, CDistributionSpecHashed::PopulateDefaultOpfamilies() tries to assemble opfamilies for all redistributable columns, which shouldn't include such types (when the GUC is set), resulting in dereferencing a nullptr. For example, int[] type has a default distr opfamily, but no legacy distr opfamily. This commit fixes the implementation of IsRedistributable() to avoid the above error. Co-authored-by: Shreedhar Hardikar <hardikar@cs.wisc.edu>
ICW concourse jobs consume lots of computing resources and make the system load is very high. and the concourse performance is very bad for the user. Creating the external concourse worker pool for icw jobs can reduce the whole concourse system workload. Add the tags attribution to the pipeline, and ensure the icw job will be placed at the external worker pool. This commit will affect the gpdb6 icw jobs on centos6/centos7/oracle7/ubuntu18.04 platforms Authored-by: Tingfang Bao <baotingfang@gmail.com>
Generated using clang-format-10 (cherry picked from commit 16b48d2)
This is intended for both local developer use and for CI. This depends on GNU parallel. One-time install: macOS: brew install parallel clang-format Debian: apt install parallel clang-format-10 To format all ORCA / GPOPT code: $ src/tools/fmt fmt To check for formatting conformance: $ src/tools/fmt chk To modify the configuration, you'll need two steps: 1. Edit clang-format.intent.yml 2. Generate the expanded configuration file: $ src/tools/fmt gen This commit also adds a formatting README, To document some of the rationale behind tooling choice. Also mention the new `README.format.md` from both the style guide and ORCA's main README. (cherry picked from commit 57b744c)
Otherwise it seems clang-format recognizes this as a multi-line preprocessor directive, strange, but preventable. (cherry picked from commit 3c67396)
Add a stage called "check-format" that runs before the fan-out of installcheck-small. While we're at it, also enforce that the full config file is generated from the intent file. This should be fairly quick. On my laptop, the `fmt chk` step takes 3 seconds. Enforcement in Concourse is forthcoming. (cherry picked from commit 54273fd)
…ps (#10872) For CTAS creating tables with non-legacy hashop distribution from tables with legacy hashops, CTAS with Orca would distribute the data according to the value of gp_use_legacy_hashops; however, it would set the table's distribution policy hashop to the value of the original table. This caused queries to give incorrect results as the distribution policy mismatched the data distribution. The data was being distributed correctly as we used the correct opfamily internally in Orca when determining the distribution spec. However, when populating the distribution policy value in the table in CTranslatorDXLToPlStmt.cpp, we were trying to derive the opclass from the opfamily. This doesn't work, and instead we need to pass the opclass from the query through Orca and to the plan statement. Hence we use the opfamily internally within Orca, and simply pass through the opclass to populate the gp_distribution_policy value correctly. Additionally, CTAS minidumps have been failing for quite a while, and subsequent changes made this a bit difficult to untangle. Since we only have ~10 mdp files, I simply regenerated most of these files and made the opfamily/opclasses arrays required as part of CMDRelationCTAS, LogicalCTAS, and PhysicalCTAS. Also, remove the `GPOS_ASSERT(NULL == mdhtacc.Find());` assert in CMDAccessor.cpp. We've been hitting this with newly created mdps since at least a couple of years, but haven't added new CTAS mdps since then. Removing the assert doesn't seem to cause any problems and it's only relevant for mdps. (cherry picked from commit a8eb84a)
(cherry picked from commit 84bff28) with some additional changes * Add a CPatternNode operator This operator can be used in patterns of xforms and it can match one of multiple regular operators. Which operators it matches depends on its match type. Right now, there is only one match type, EmtMatchInnerOrLeftOuterJoin, that matches a logical inner join or a logical left outer join. * Add 2 new xforms for index apply The new xforms use the new CPatternNode for a patterns of the form CPatternNode(Leaf, Tree, Tree) The CPatternNode matches both an inner and a left outer join. The Tree for the right child can contain arbitrary operators. To avoid an explosion of the search space, we add two conditions to the xform: First, the xform is applied only once, not for all bindings of the Tree node. Second, the xform is applied only if the right child has a "join depth" of 1, excluding any children that are complex and wouldn't satisfy the conditions of this xform anyway. * Remove 16 obsolete xforms and replace them with 2 new ones To remove xforms, we have to add a mechanism to skip unused xform ids, to preserve the ids of the remaining xforms that are used in trace flags. Our array of xforms now allows for holes. * Changes to unit test programs Updated the test programs to tolerate holes in the xform array. Used a new xform instead of one of the removed ones. * MDP changes * ICG changes * Fixes for ICG failures in join and aggregates tests The new xform allows additional plans that are chosen in the explain output. It also surfaced a bug where we can't eliminate a groupby that sits on top of a CLogicalIndexGet, because the index get doesn't derive a key set. * Support for project nodes in index nested loop joins When generating required distribution specs for its children, CPhysicalInnerIndexNLJoin will start with its inner child and send it an ANY required distribution spec. It will then force the outer child to match the inner's distribution spec (or require a broadcast on the outer). Now, assume we have CPhysicalComputeScalar as the inner child. This node, in CPhysicalComputeScalar::PdsRequired will currently require its child to be REPLICATED (opt request 1) or SINGLETON (opt request 0), if the expression has any outer references. This won't work, since the underlying table has neither of these distribution schemes and since we don't want any motions between the index join and the index get. This commit changes the behavior of a CPhysicalComputeScalar. If it senses that it is part of an index nested loop join, it will just propagate the required distribution spec from the parent. How does it sense that? By the required ANY distribution spec that allows outer references. This request is generated in only two places: CPhysicalInnerIndexNLJoin::PdsRequired and CPhysicalLeftOuterIndexNLJoin::PdsRequired, so it is only used in the context of an index NLJ. This behavior is similar to what the CPhysicalFilter node does, the other node allowed between an index NLJ and the get. * Adding a check for grouping columns Allow groupby under the right side of an index join only if the groupby can be co-located with the get underneath. This happens when the grouping columns are a superset of the distribution columns, since then every group will be entirely contained in a single segment. * History of this commit This is the second attempt at this commit, after having to revert earlier commits 24671b5 and 17da102. In addition to the first commit, we had to add a check for the groupby columns to be a superset of the distribution columns (see item above) and we also fixed several diffs in tests. Co-authored-by: David Kimura <dkimura@vmware.com> Co-authored-by: Hans Zeller <hzeller@vmware.com> (cherry picked from commit 84bff28)
Planner was choosing a different join order for different builds. Spreading out the values of the smaller table, so that for a hash join, the two tables will be of different size. That should make choosing a hash join order more clear.
Earlier the output filename for the test check_orphaned_toastrels was partitioned_tables.txt, so updated that to represent toast tables
With the ifaddrs utility, we excluded ip addresses on the loopback interface which caused regression causing replication entries to be not populated for such interfaces causing gpaddmirrors and gpinitstandby to fail. Routable IP addresses can be assigned to the loopback interface, and this case was not considered earlier. This commit fixes the issues by allowing all loopback addresses except 127.0.0.1 and ::1 address
In a previous commit (df5f06d), we added code to fall back gracefully when a subquery select list contains a single outer ref that is not part of an expression, such as in select * from foo where a is null or a = (select foo.b from bar) This commit adds a fix that allows us to handle such queries in ORCA by adding a project in the translator that will echo the outer ref from within the subquery, and using that projected value in the select list of the subquery. This ensures that we use a NULL value for the scalar subquery in the expression for the outer ref when the subquery returns no rows. Also note that this is still skipped for grouping cols in the target list. This was done to avoid regression for certain queries, such as: select * from A where not exists (select sum(C.i) from C where C.i = A.i group by a.i); ORCA is currently unable to decorrelate sub-queries that contain project nodes, So, a `SELECT 1` in the subquery would also cause this regression. In the above query, the parser adds `a.i` to the target list of the subquery, that would get an echo projection (as described above), and thus would prevent decorelation by ORCA. For this reason, we decided to maintain existing behavior until ORCA is able to handle projections in subqueries better. Also, regenerate 6 MDPs with outer refs in their project lists Regenerating the MDPs for 6 test cases that were identified having outer refs in their project lists. These test cases will go through the fix in the translator and most of them will generate an extra project in the DXL query section. Some of them now also have an extra project in the plan section. Co-authored-by: Hans Zeller <hzeller@pivotal.io> Co-authored-by: Shreedhar Hardikar <shardikar@pivotal.io>
The function combines two project nodes - one underneath the other. The
assertion checks if the newly created project uses only available
colrefs. However, it didn't include the outer refs in the upper project
node.
This commit fixes the assert highlighted by the following case:
create table foo(a int, b int);
create table bar(a int, b int);
select *
from foo
where a is null or
a = (select foo.a+1
from (select a+2 a2, b+2 b2
from bar) l2
where l2.b2 = 5);
Co-authored-by: Hans Zeller <hzeller@pivotal.io>
We had several issues on 5x and 6x that shows the resource group wait queue is somehow corrupted, this commit add some checks and PANIC at the earliest to help debugging.
We used to free the workfile_set when all its files are closed. But in some cases, we use the workfile_set for the whole node execution process. So, we should pin the workfile_set to avoid unexcepted free operation. And manually remove it once we are sure we don't want to use it anymore. Otherwise, the freed workfile_set may be allocated to others. This fixes the issue: https://github.com/greenplum-db/gpdb/issues/10855. Also, take care of the empty workfile_set, free it when applicable. Distinguish by the workfile_set life cycle, the workfile_set could be class into several classes. 1. workfile_set for a temporary workfile with interXact false(see buffile.c). Once the file closed, the workfile_set will also get freed. And if current transaction aborted, the workfile's resource owner is responsible for closing it if the workfile is leaked, which also removes the workfile_set. 2. workfile_set for a temporary workfile with interXact true(see buffile.c). Once the file closed, the workfile_set will also get removed. Since the workfile doesn't register to a resource owner, WorkFileLocalCtl is responsible for removing its corresponding workfile_set in AtProcExit_WorkFile(which gets called when process exit). 3. workfile_set is pinned and saved to prevent unexpected free operation. When the workfile_set gets pinned, after all the file in the workfile_set gets closed, we don't free the workfile_set, cause we may register new workfile later. Call workfile_mgr_close_set to explicitly remove the workfile_set. To deal with aborting, since the resource owner of the workfiles in the workfile_set will close the workfiles, and this operation will not remove workfile_set when no file exists in the workfile_set. We have to free these kinds of workfile_sets when transaction ends in AtEOXact_WorkFile. If the workfile_set contains interXact workfiles, we still rely on WorkFileLocalCtl to remove workfile_sets in AtProcExit_WorkFile. But currently, we don't have this kind of usage. NOTE: if a workfile_set gets created but no workfile register into it(in case of error, RegisterFileWithSet not called), it will never have a chance to be removed automatically through file close. Then we have to check and remove these workfile_sets when the transaction end to prevent workfile_set overflow in AtEOXact_WorkFile. Co-authored-by: Gang Xiong <gxiong@pivotal.io> (cherry picked from commit 5b147fb)
This includes the commit on master, 2 back branches, and the standalone gporca repository. (cherry picked from commit 7ab6b7a)
Collaborator
|
@darthunix code formatting from 4a37ae9 conflicts with our patch which downgrade planner for replicated table and volatile functions: https://arenadata.atlassian.net/browse/ADBDEV-918 |
|
I have created a ADBDEV-1091 branch that resolved the conflict. If everyone is satisfied with a conflict resolution there I can push these changes to a target |
Stolb27
pushed a commit
that referenced
this pull request
Feb 15, 2024
Per pull request #121, from Felix Geisendörfer. Backpatch-through: 11
RekGRpth
added a commit
that referenced
this pull request
Dec 3, 2025
Fix escaping for perfmon metrics export What occurs? Whenever new log rows, containing database names with some special characters, were tried to be uploaded from _queries_history.dat file to the queries_history external table an error considering the size of a db column would occur. It would prevent any new logs to be loaded whatsoever. Why it occurs? The db column has a type of VARCHAR(64), thus it means that a larger string was tried to be put. Obvious reason for this - incorrect escaping or lack of such whatsoever. Only two symbols were observed to lead to errors: " and |. In case of a pipe - it leads to another error about incorrect row structure, which is logical, as pipes are used as delimiters inside the file. How do we fix it? Before the patch (1f67d39) db field used to be written to _queries_history.dat without double quotes, so no escaping was present. Now it has it, as we enclose every database name in double quotes. Also, we double the already present in database name double quotes. Does it fix the problem? Yes, as now we escape the whole database name string with all of its special (or not) characters - the very same method is used for the SQL query command and it works. Doubling double quotes is needed as we need to escape the quotes to not to end the string to early. What was changed? Minor code additions for escaping inside the src/backend/gpmon/gpmon.c Tests? New BDD auto test to check that before mentioned logs are added to queries_history correctly (as they appear there). But, not all symbols can be auto tested using present testing functions: ', | and UTF-8 symbols. Errors occur at different steps of a test for different type of symbol and are connected to the way commands are issued to the DBSM. Nevertheless, during hand testing these symbols passed. Observations? dbuser column also is not escaped, but I have not managed to recreate the same issue with it. Yet, it may be worth to add escaping to it in future, but now it seems like an extremely edge case scenario. Ticket: ADBDEV-7272 --------- Co-authored-by: Vladimir Sarmin <v.sarmin@arenadata.io> Co-authored-by: Georgy Shelkovy <g.shelkovy@arenadata.io>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
10 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.