Open
Conversation
edbmanniwood
commented
Sep 25, 2025
src/backend/utils/adt/ruleutils.c
Outdated
| * "allow_in_place_tablespaces = true" and "LOCATION ''", | ||
| * path will begin with "pg_tblspc/". In that case, show | ||
| * "LOCATION ''" as the user originally specified. */ | ||
| if (strncmp("pg_tblspc/", path, 10) == 0) |
Owner
Author
There was a problem hiding this comment.
Is this the correct way to check for a prefix (a.k.a. startsWith) in C?
akshay-joshi
suggested changes
Oct 3, 2025
| /* Look up the tablespace in pg_tablespace */ | ||
| tuple = SearchSysCache1(TABLESPACEOID, ObjectIdGetDatum(tspaceoid)); | ||
|
|
||
| Assert(HeapTupleIsValid(tuple)); |
There was a problem hiding this comment.
Instead of using Assert, check HeapTupleIsValid(tuple), and if it’s invalid, return with ereport
Comment on lines
+13798
to
+13835
| if (opts->random_page_cost > 0) | ||
| { | ||
| appendStringInfo(&buf, "random_page_cost = %g", | ||
| opts->random_page_cost); | ||
| needcomma = true; | ||
| } | ||
|
|
||
| if (opts->seq_page_cost > 0) | ||
| { | ||
| if (needcomma) | ||
| appendStringInfo(&buf, ", seq_page_cost = %g", | ||
| opts->seq_page_cost); | ||
| else | ||
| appendStringInfo(&buf, "seq_page_cost = %g", | ||
| opts->seq_page_cost); | ||
| needcomma = true; | ||
| } | ||
|
|
||
| if (opts->effective_io_concurrency > 0) | ||
| { | ||
| if (needcomma) | ||
| appendStringInfo(&buf, ", effective_io_concurrency = %d", | ||
| opts->effective_io_concurrency); | ||
| else | ||
| appendStringInfo(&buf, "effective_io_concurrency = %d", | ||
| opts->effective_io_concurrency); | ||
| needcomma = true; | ||
| } | ||
|
|
||
| if (opts->maintenance_io_concurrency > 0) | ||
| { | ||
| if (needcomma) | ||
| appendStringInfo(&buf, ", maintenance_io_concurrency = %d", | ||
| opts->maintenance_io_concurrency); | ||
| else | ||
| appendStringInfo(&buf, "maintenance_io_concurrency = %d", | ||
| opts->maintenance_io_concurrency); | ||
| } |
There was a problem hiding this comment.
I am not a C expert; others can comment, but,
Can this logic be changed to below:
if (opts->random_page_cost > 0)
appendStringInfo(&buf, "random_page_cost = %g, ",
opts->random_page_cost);
if (opts->seq_page_cost > 0)
appendStringInfo(&buf, "seq_page_cost = %g, ",
opts->seq_page_cost);
if (opts->effective_io_concurrency > 0)
appendStringInfo(&buf, "effective_io_concurrency = %d, ",
opts->effective_io_concurrency);
if (opts->maintenance_io_concurrency > 0)
appendStringInfo(&buf, "maintenance_io_concurrency = %d",
opts->maintenance_io_concurrency);
/* Removing trailing comma and space */
if (buf.data[buf.len - 2] == ',' && buf.data[buf.len - 1] == ' ')
{
buf.len -= 2;
buf.data[buf.len] = '\0'; // Null-terminate the modified string
}
edbmanniwood
pushed a commit
that referenced
this pull request
Oct 28, 2025
truncate_useless_pathkeys() seems to have neglected to account for PathKeys that might be useful for WindowClause evaluation. Modify it so that it properly accounts for that. Making this work required adjusting two things: 1. Change from checking query_pathkeys to check sort_pathkeys instead. 2. Add explicit check for window_pathkeys For #1, query_pathkeys gets set in standard_qp_callback() according to the sort order requirements for the first operation to be applied after the join planner is finished, so this changes depending on which upper planner operations a particular query needs. If the query has window functions and no GROUP BY, then query_pathkeys gets set to window_pathkeys. Before this change, this meant PathKeys useful for the ORDER BY were not accounted for in queries with window functions. Because of #1, #2 is now required so that we explicitly check to ensure we don't truncate away PathKeys useful for window functions. Author: David Rowley <dgrowleyml@gmail.com> Discussion: https://postgr.es/m/CAApHDvrj3HTKmXoLMbUjTO=_MNMxM=cnuCSyBKidAVibmYPnrg@mail.gmail.com
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
2 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.
This uses Nishant Sharma's code, which does a nicer job of only showing the tablespace options when the user explicitly set them. (My earlier code used the
get_tablespace_page_costs()and related functions, which failed to make that distinction.)Tests have now been added. We take advantage of the special
allow_in_place_tablespacesdeveloper GUC.