Re: psql: \dl+ to list large objects privileges
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi, There is something I forgot to mention in my previous review. Typically when describing a thing in psql, it is the column "Description" that belongs in the verbose version. For example listing indexes produces: List of relations Schema | Name| Type | Owner | Table and the verbose version: List of relations Schema | Name| Type | Owner | Table | Persistence | Access method | Size| Description Since '\dl' already contains description, it might be worthwhile to consider to add the column `Access privileges` without introducing a verbose version. Thoughts? Cheers, //Georgios
Re: psql: \dl+ to list large objects privileges
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
Hi,
thank you for the patch, I personally think it is a useful addition and thus it
gets my vote. However, I also think that the proposed code will need some
changes.
On a high level I will recommend the addition of tests. There are similar tests
present in:
./src/test/regress/sql/psql.sql
I will also inquire as to the need for renaming the function `do_lo_list` to
`listLargeObjects` and its move to describe.c. from large_obj.c. In itself it is
not necessarily a blocking point, though it will require some strong arguments
for doing so.
Applying the patch, generates several whitespace warnings. It will be helpful
if those warnings are removed.
The patch contains:
case 'l':
- success = do_lo_list();
+ success = listLargeObjects(show_verbose);
It might be of some interest to consider in the above to check the value of the
next character in command or emit an error if not valid. Such a pattern can be
found in the same switch block as for example:
switch (cmd[2])
{
case '\0':
case '+':
success = ...
break;
default:
status = PSQL_CMD_UNKNOWN;
break;
}
The patch contains:
else if (strcmp(cmd + 3, "list") == 0)
- success = do_lo_list();
+ success = listLargeObjects(false);
+
+ else if (strcmp(cmd + 3, "list+") == 0)
+ success = listLargeObjects(true);
In a fashion similar to `exec_command_list`, it might be interesting to consider
expressing the above as:
show_verbose = strchr(cmd, '+') ? true : false;
else if (strcmp(cmd + 3, "list") == 0
success = do_lo_list(show_verbose);
Thoughts?
Cheers,
//Georgios
Re: Allow batched insert during cross-partition updates
Hi, thanks for the patch. I had a first look and played around with the code. The code seems clean, complete, and does what it says on the tin. I will need a bit more time to acclimatise with all the use cases for a more thorough review. I small question though is why expose PartitionTupleRouting and not add a couple of functions to get the necessary info? If I have read the code correctly the only members actually needed to be exposed are num_partitions and partitions. Not a critique, I am just curious. Cheers, //Georgios
Re: [PATCH] postgres-fdw: column option to override foreign types
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi, Thanks for the patch. I am afraid I will have to :-1: this patch. Of course it is possible that I am wrong, so please correct me if you, or any other reviewers, think so. The problem that is intended to be solved, upon closer inspection seems to be a conscious design decision rather than a problem. Even if I am wrong there, I am not certain that the proposed patch covers all the bases with respect to collations, build-in types, shipability etc for simple expressions, and covers any of more complicated expressions all together. As for the scenario which prompted the patch, you wrote, quote: The real scenario that prompted it is a tickets table with status, priority, category, etc. enums. We don't have plans to modify them any time soon, but if we do it's got to be coordinated and deployed across two databases, all so we can use what might as well be a text column in a single WHERE clause. Since foreign tables can be defined over subsets of columns, reordered, and names changed, a little opt-in flexibility with types too doesn't seem misplaced. end quote. I will add that creating a view on the remote server with type flexibility that you wish and then create foreign tables against the view, might address your problem. Respectfully, //Georgios
Re: GROUP BY DISTINCT
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi, this is a useful feature, thank you for implementing. I gather that it follows the standard, if so, then there are definitely no objections from me. The patch in version 2, applies cleanly and passes all the tests. It contains documentation which seems correct to a non native speaker. As a minor gripe, I would note the addition of list_int_cmp. The block + /* Sort each groupset individually */ + foreach(cell, result) + list_sort(lfirst(cell), list_int_cmp); Can follow suit from the rest of the code, and define a static cmp_list_int_asc(), as indeed the same patch does for cmp_list_len_contents_asc. This is indeed point of which I will not hold a too strong opinion. Overall :+1: from me. I will be bumping to 'Ready for Committer' unless objections.
Re: Error on failed COMMIT
Hi, this patch fails on the cfbot yet it has received an update during the current CF. I will move it to the next CF and mark it there as Waiting on Author. Cheers, Georgios The new status of this patch is: Needs review
Re: Display individual query in pg_stat_activity
This patch fails in the cfbot for quite some time now. I shall close it as Return With Feedback and not move it to the next CF. Please feel free to register an updated version afresh for the next CF. Cheers, //Georgios
Re: Supporting = operator in gin/gist_trgm_ops
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: tested, passed Spec compliant: not tested Documentation:not tested Hi, this patch implements a useful and missing feature. Thank you. It includes documentation, which to a non-native speaker as myself seems appropriate. It includes comprehensive tests that cover the implemented cases. In the thread Alexander has pointed out, quote: "It would be more efficient to generate trigrams for equal operator using generate_trgm() instead of generate_wildcard_trgm()" I will echo the sentiment, though from a slightly different and possibly not as important point of view. The method used to extract trigrams from the query should match the method used to extract trigrams from the values when they get added to the index. This is gin_extract_value_trgm() and is indeed using generate_trgm(). I have no opinion over Alexander's second comment regarding costing. I change the status to 'Waiting on Author', but please feel free to override my opinion if you feel I am wrong and reset it to 'Needs review'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: SQL-standard function body
Hi, I noticed that this patch fails on the cfbot. For this, I changed the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: Error on failed COMMIT
Hi, I noticed that this patch fails on the cfbot. For this, I changed the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: WIP: System Versioned Temporal Table
Hi, just a quick comment that this patch fails on the cfbot. Cheers, //Georgios
Re: Allow an alias to be attached directly to a JOIN ... USING
Hi, I noticed that this patch fails on the cfbot. For this, I changed the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: [PATCH] Add extra statistics to explain for Nested Loop
Hi, I noticed that this patch fails on the cfbot. For this, I changed the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: Get memory contexts of an arbitrary backend process
Hi, I noticed that this patch fails on the cfbot. For this, I changed the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: Display individual query in pg_stat_activity
Hi, I noticed that this patch is failing on the cfbot. For this, I changed the status to: 'Waiting on Author' Cheers, Georgios The new status of this patch is: Waiting on Author
Re: Add session statistics to pg_stat_database
Hi, I noticed that the cfbot fails for this patch. For this, I am setting the status to: 'Waiting on Author'. Cheers, //Georgios The new status of this patch is: Waiting on Author
Re: Strange behavior with polygon and NaN
Hi, apologies for the very, very late reply to your fixes. You have answered/addressed all my questions concerns. The added documentation reads well, at least to a non native English speaker. The patch still applies and as far as I can see the tests are passing. It gets my :+1: and I am changing the status to "Ready for Committer". For what little is worth, I learned a lot from this patch, thank you. Cheers, Georgios The new status of this patch is: Ready for Committer
Re: Error on failed COMMIT
Hi, thank you for your contribution. I did notice that the cfbot [1] is failing for this patch. Please try to address the issue for the upcoming commitfest. Cheers, //Georgios [1] http://cfbot.cputube.org/dave-cramer.html
Re: pg_upgrade analyze script
Hi, thank you for you contribution. I did notice that the cfbot [1] is failing for this patch. Please try to address the issues for the upcoming Commitfest. Cheers, //Georgios [1] http://cfbot.cputube.org/magnus-hagander.html
Re: Extending range type operators to cope with elements
Hi, thank you for your contribution. I did notice that the cfbot [1] is failing for this patch. Please try to address the issues if you can for the upcoming commitfest. Cheers, //Georgios [1] http://cfbot.cputube.org/esteban-zimanyi.html
Re: Consistent error reporting for encryption/decryption in pgcrypto
Hi, thank you for your contribution. I did notice that the cfbot [1] is not failing for this patch. Cheers, //Georgios [1] http://cfbot.cputube.org/daniel-gustafsson.html
Re: shared-memory based stats collector
Hi, I noticed that according to the cfbot this patch no longer applies. As it is registered in the upcoming commitfest, it would be appreciated if you could rebase it. Cheers, //Georgios
Re: New default role- 'pg_read_all_data'
Hi, this patch is in "Ready for committer" state and the Cfbot is happy. Is there any committer that is available for taking a look at it? Cheers, //Georgios - CFM 2020-11
Re: v13: show extended stats target in \d
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi, I will humbly disagree with the current review. I shall refrain from changing the status though because I do not have very strong feeling about it. I am in agreement that it is a helpful feature and as implemented, the result seems to be the desired one. However the patch contains: - " 'm' = any(stxkind) AS mcv_enabled\n" + " 'm' = any(stxkind) AS mcv_enabled,\n" + " %s" "FROM pg_catalog.pg_statistic_ext stat " "WHERE stxrelid = '%s'\n" "ORDER BY 1;", + (pset.sversion >= 13 ? "stxstattarget\n" : "-1\n"), oid); This seems to be breaking a bit the pattern in describeOneTableDetails(). First, it is customary to write the whole query for the version in its own block. I do find this pattern rather helpful and clean. So in my humble opinion, the ternary expression should be replaced with a distinct if block that would implement stxstattarget. Second, setting the value to -1 as an indication of absence breaks the pattern a bit further. More on that bellow. + if (strcmp(PQgetvalue(result, i, 8), "-1") != 0) + appendPQExpBuffer(, " STATISTICS %s", + PQgetvalue(result, i, 8)); + In the same function, one will find a bit of a different pattern for printing the statistics, e.g. if (strcmp(PQgetvalue(result, i, 7), "t") == 0) I will again hold the opinion that if the query gets crafted a bit differently, one can inspect if the table has `stxstattarget` and then, go ahead and print the value. Something in the lines of: if (strcmp(PQgetvalue(result, i, 8), "t") == 0) appendPQExpBuffer(, " STATISTICS %s", PQgetvalue(result, i, 9)); Finally, the patch might be more complete if a note is added in the documentation. Have you considered adding something in doc/src/sgml/ref/psql-ref.sgml? If no, will you consider it? If yes, why did you discard it? Regards, Georgios
Re: New default role- 'pg_read_all_data'
The following review has been posted through the commitfest application: make installcheck-world: tested, passed Implements feature: not tested Spec compliant: not tested Documentation:not tested Version 2 of the patch, implements a useful feature. Based on the mailing list discussion, it is also a feature that the community desires. The code seems to be correct and it follows the style. The patch comes complete with tests and documentation. As a non native English speaker, I did not notice any syntactical or grammatical errors in the documentation. Yet it should not mean a lot. As far as I am concerned, this version of the patch is ready for a committer. Please feel free to contest my review, if you think I am wrong. The new status of this patch is: Ready for Committer
Re: New default role- 'pg_read_all_data'
Thank you for the patch. My high level review comment: The patch seems to be implementing a useful and requested feature. The patch applies cleanly and passes the basic regress tests. Also the commitfest bot is happy. A first pass at the code, has not revealed any worthwhile comments. Please allow me for a second and more thorough pass. The commitfest has hardly started after all. Also allow me a series of genuine questions: What would the behaviour be with REVOKE? In a sequence similar to: GRANT ALL ON ... REVOKE pg_read_all_data FROM ... What privileges would the user be left with? Would it be possible to end up in the same privilege only with a GRANT command? Does the above scenario even make sense? Regards,
Re: [PATCH] Finally split StdRdOptions into HeapOptions and ToastOptions
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Hi, thank you for the patch. It applies cleanly, compiles and passes check, check-world. I feel as per the discussion, this is a step to the right direction yet it does not get far enough. From experience, I can confirm that dealing with reloptions in a new table AM is somewhat of a pain. Ultimately, reloptions should be handled by the table AM specific code. The current patch does not address the issue. Yet it does make the issue easier to address by clearing up the current state. If you allow me, I have a couple of comments. - saveFreeSpace = RelationGetTargetPageFreeSpace(relation, - HEAP_DEFAULT_FILLFACTOR); + if (IsToastRelation(relation)) + saveFreeSpace = ToastGetTargetPageFreeSpace(); + else + saveFreeSpace = HeapGetTargetPageFreeSpace(relation); For balance, it does make some sense for ToastGetTargetPageFreeSpace() to get relation as an argument, similarly to HeapGetTargetPageFreeSpace(). Also, this pattern is repeated in four places, maybe the branch can be moved inside a macro or static inline instead? - /* Retrieve the parallel_workers reloption, or -1 if not set. */ - rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1); + /* +* Retrieve the parallel_workers for heap and mat.view relations. +* Use -1 if not set, or if we are dealing with other relation kinds +*/ + if (relation->rd_rel->relkind == RELKIND_RELATION || + relation->rd_rel->relkind == RELKIND_MATVIEW) + rel->rel_parallel_workers = RelationGetParallelWorkers(relation, -1); + else + rel->rel_parallel_workers = -1; If the comment above is agreed upon, then it makes a bit of sense to apply the same here. The expression in the branch is already asserted for in macro, why not switch there and remove the responsibility from the caller? Any thoughts on the above? Cheers, Georgios The new status of this patch is: Waiting on Author
Re: Using Valgrind to detect faulty buffer accesses (no pin or buffer content lock held)
As a general overview, the series of patches in the mail thread do match their description. The addition of the stricter, explicit use of instrumentation does improve the design as the distinction of the use cases requiring a pin or a lock is made more clear. The added commentary is descriptive and appears grammatically correct, at least to a non native speaker. Unfortunately though, the two bug fixes do not seem to apply. Also, there is a small issue regarding the process, not the content of the patches. In CF app there is a latest attachment (v3-0002-Add-nbtree-Valgrind-buffer-lock-checks.patch) which does not appear in the mail thread. Before changing the status, I will kindly ask for the complete latest series that applies in the mail thread. The new status of this patch is: Waiting on Author
Re: Refactor compile-time assertion checks for C/C++
Thank you for updating the status of the issue. I have to admit that I completely missed the CF bot. Lesson learned. For whatever is worth, my previous comment that the patch improves readability also applies to the updated version of the patch. The CF bot seems happy now, which means that your assessment as to the error and fix are correct. So :+1: from me.
Re: ALTER TEXT SEARCH DICTIONARY tab completion
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
It looks good and does what it says on the tin.
One minor nitpick I feel I should add is that for completeness and
balance the equivalent `CREATE TEXT SEARCH DICTIONARY` should
get the same treatment.
Maybe something along the lines of:
- else if (Matches("CREATE", "TEXT", "SEARCH", "CONFIGURATION", MatchAny))
+ else if (Matches("CREATE", "TEXT", "SEARCH",
"DICTIONARY|CONFIGURATION", MatchAny))
Re: Refactor compile-time assertion checks for C/C++
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested In my humble opinion the patch improves readability, hence gets my +1. No further suggestions. Passing on to a committer to judge further. The new status of this patch is: Ready for Committer
Re: Duplicate Workers entries in some EXPLAIN plans
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested > Ah, I see. I think I got that from ExplainPrintSettings. I've > corrected my usage--thanks for pointing it out. I appreciate the > effort to maintain a consistent style. Thanks, I am just following the reviewing guide to be honest. > Also, reviewing my code again, I noticed that when I moved the general > worker output earlier, I missed part of the merge conflict: Right. I thought that was intentional. > which ignores the es->hide_workers flag (it did not fail the tests, > but the intent is pretty clear). I've corrected this in the current > patch. Noted and appreciated. > I also noticed that we can now handle worker buffer output more > consistently across TEXT and structured formats, so I made that small > change too: Looks good. > I think the "producing plan output for a worker" process is easier to > reason about now, and while it changes TEXT format worker output > order, the other changes in this patch are more drastic so this > probably does not matter. > > I've also addressed the other feedback above, and reworded a couple of > comments slightly. Thanks for the above. Looks clean, does what it says in the tin and solves a problem that needs solving. All relevant installcheck-world pass. As far as I am concerned, I think it is ready to be sent to a committer. Updating the status accordingly. The new status of this patch is: Ready for Committer
Re: Duplicate Workers entries in some EXPLAIN plans
>> + int num_workers = planstate->worker_instrument->num_workers;
>> + int n;
>> + worker_strs = (StringInfo *) palloc0(num_workers *
>> sizeof(StringInfo));
>> + for (n = 0; n < num_workers; n++) {
>>
>> I think C99 would be better here. Also no parenthesis needed.
>
>
> Pardon my C illiteracy, but what am I doing that's not valid C99 here?
My bad, I should have been more clear. I meant that it is preferable to use
the C99 standard which calls for declaring variables in the scope that you
need them. In this case, 'n' is needed only in the for loop, so something like
for (int n = 0; n < num_workers; n++)
is preferable. To be clear, your code was perfectly valid. It was only the
style I was referring to.
>> + for (n = 0; n < w->num_workers; ++n)
>>
>> I think C99 would be better here.
>
>
> And here (if it's not the same problem)?
Exactly the same as above.
>> int indent; /* current indentation level */
>> List *grouping_stack; /* format-specific grouping state */
>> + boolprint_workers; /* whether current node has worker metadata
>> */
>>
>> Hmm.. commit introduced `hide_workers` in the struct. Having
>> both
>> names in the struct so far apart even seems a bit confusing and sloppy. Do
>> you
>> think it would be possible to combine or rename?
>
>
> I noticed that. I was thinking about combining them, but
> "hide_workers" seems to be about "pretend there is no worker output
> even if there is" and "print_workers" is "keep track of whether or not
> there is worker output to print". Maybe I'll rename to
> "has_worker_output"?
The rename sounds a bit better in my humble opinion. Thanks.
Re: Duplicate Workers entries in some EXPLAIN plans
The following review has been posted through the commitfest application:
make installcheck-world: not tested
Implements feature: not tested
Spec compliant: not tested
Documentation:not tested
> TEXT format was tricky due to its inconsistencies, but I think I have
> something working reasonably well. I added a simple test for TEXT
> format output as well, using a similar approach as the JSON format
Great!
> test, and liberally regexp_replacing away any volatile output. I
> suppose in theory we could do this for YAML, too, but I think it's
> gross enough not to be worth it, especially given the high similarity
> of all the structured outputs.
Agreed, what is in the patch suffices. Overall great work, a couple of
minor nitpicks if you allow me.
+ /* Prepare per-worker output */
+ if (es->analyze && planstate->worker_instrument) {
Style, parenthesis on its own line.
+ int num_workers = planstate->worker_instrument->num_workers;
+ int n;
+ worker_strs = (StringInfo *) palloc0(num_workers * sizeof(StringInfo));
+ for (n = 0; n < num_workers; n++) {
I think C99 would be better here. Also no parenthesis needed.
+ worker_strs[n] = makeStringInfo();
+ }
+ }
@@ -1357,6 +1369,58 @@ ExplainNode(PlanState *planstate, List *ancestors,
ExplainPropertyBool("Parallel Aware", plan->parallel_aware, es);
}
+ /* Prepare worker general execution details */
+ if (es->analyze && es->verbose && planstate->worker_instrument)
+ {
+ WorkerInstrumentation *w = planstate->worker_instrument;
+ int n;
+
+ for (n = 0; n < w->num_workers; ++n)
I think C99 would be better here.
+ {
+ Instrumentation *instrument = >instrument[n];
+ double nloops = instrument->nloops;
- appendStringInfoSpaces(es->str, es->indent * 2);
- if (n > 0 || !es->hide_workers)
- appendStringInfo(es->str, "Worker %d: ", n);
+ if (indent)
+ {
+ appendStringInfoSpaces(es->str, es->indent * 2);
+ }
Style: No parenthesis needed
- if (opened_group)
- ExplainCloseGroup("Workers", "Workers", false, es);
+ /* Show worker detail */
+ if (planstate->worker_instrument) {
+ ExplainFlushWorkers(worker_strs,
planstate->worker_instrument->num_workers, es);
}
Style: No parenthesis needed
+* just indent once, to add worker info on the next worker line.
+*/
+ if (es->str == es->root_str)
+ {
+ es->indent += es->format == EXPLAIN_FORMAT_TEXT ? 1 : 2;
+ }
+
Style: No parenthesis needed
+ ExplainCloseGroup("Workers", "Workers", false, es);
+ // do we have any other cleanup to do?
This comment does not really explain anything. Either remove
or rephrase. Also C style comments.
+ es->print_workers = false;
+}
int indent; /* current indentation level */
List *grouping_stack; /* format-specific grouping state */
+ boolprint_workers; /* whether current node has worker metadata */
Hmm.. commit introduced `hide_workers` in the struct. Having both
names in the struct so far apart even seems a bit confusing and sloppy. Do you
think it would be possible to combine or rename?
+extern void ExplainOpenWorker(StringInfo worker_str, ExplainState *es);
+extern void ExplainCloseWorker(ExplainState *es);
+extern void ExplainFlushWorkers(StringInfo *worker_strs, int num_workers,
ExplainState *es);
No need to expose those, is there? I feel there should be static.
Awaiting for answer or resolution of these comments to change the status.
//Georgios
Re: Duplicate Workers entries in some EXPLAIN plans
> Sounds good, I'll try that format. Any idea how to test YAML? With the > JSON format, I was able to rely on Postgres' own JSON-manipulating > functions to strip or canonicalize fields that can vary across > executions--I can't really do that with YAML. Yes, this approach was clear in the patch and works great with Json. Also you are correct, this can not be done with YAML. I spend a bit of time to look around and I could not find any tests really on yaml format. > Or should I run EXPLAIN > with COSTS OFF, TIMING OFF, SUMMARY OFF and assume that for simple > queries the BUFFERS output (and other fields I can't turn off like > Sort Space Used) *is* going to be stable? I have to admit with the current diff tool used in pg_regress, this is not possible. I am pretty certain that it *is not* going to be stable. Not for long anyways. I withdraw my suggestion for YAML and currently awaiting for TEXT format only.
Re: Duplicate Workers entries in some EXPLAIN plans
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: not tested Spec compliant: not tested Documentation:not tested The current version of the patch (v2) applies cleanly and does what it says on the box. > Any thoughts on what we should do with text mode output (which is untouched right now)? The output Andres proposed above makes sense to me, but I'd like to get more input. Nothing to add beyond what is stated in the thread. > Sort Method: external merge Disk: 4920kB > Buffers: shared hit=682 read=10188, temp read=1415 written=2101 > Worker 0: actual time=130.058..130.324 rows=1324 loops=1 >Sort Method: external merge Disk: 5880kB >Buffers: shared hit=337 read=3489, temp read=505 written=739 > Worker 1: actual time=130.273..130.512 rows=1297 loops=1 >Buffers: shared hit=345 read=3507, temp read=505 written=744 >Sort Method: external merge Disk: 5920kB This proposal seems like a fitting approach. Awaiting for v3 which will include the text version. May I suggest a format yaml test case? Just to make certain that no regressions occur in the future. Thanks,
Re: Duplicate Workers entries in some EXPLAIN plans
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested This is a high level review only. However, seeing that there is a conflict and does not merge cleanly after commit , I return to Author. To be fair the resolution seems quite straight forward and I took the liberty of applying the necessary changes to include the new element of ExplainState introduced in the above commit, namely hide_workers. However since the author might have a different idea on how to incorporate this change I leave it up to him. Another very high level comment is the introduction of a new test file, namely explain. Seeing `explain.sql` in the tests suits, personally and very opinion based, I would have expected the whole spectrum of the explain properties to be tested. However only a slight fraction of the functionality is tested. Since this is a bit more of a personal opinion, I don't expect any changes unless the author happens to agree. Other than these minor nitpick, the code seems clear, concise and does what it says on the box. It follows the comments in the discussion thread(s) and solves a real issue. Please have a look on how commit affects this patch and rebase. The new status of this patch is: Waiting on Author
Re: CPU costs of random_zipfian in pgbench
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested Version 3 of the patch looks ready for committer. Thank you for taking the time to code! The new status of this patch is: Ready for Committer
Re: CPU costs of random_zipfian in pgbench
The following review has been posted through the commitfest application: make installcheck-world: not tested Implements feature: not tested Spec compliant: not tested Documentation:not tested For whatever it is worth, the patch looks good to me. A minor nitpick would be to use a verb in the part: `cost when the parameter in (0, 1)` maybe: `cost when the parameter's value is in (0, 1)` or similar. Apart from that, I would suggest it that the patch could be moved to Waiting for Author state.
Re: Adding a TAP test checking data consistency on standby with minRecoveryPoint
Hi, you are right in saying that my comment didn't offer much of a constructive explanation. Apologies for that. To the issue at hand. Tests were run in the same manner as in all other cases and the test in question was the only one to fail in the whole tree. By looking a bit closer to the error, the culprit seems to be the missing installation of `pageinspect`, as it was correctly pointed out. I did notice before sending the first message in the thread that `contrib/pageinspect` was added to the `EXTRA_INSTALL` variable and I *assumed* that it should be properly installed to the system. However that was not the case. Without being a recursive Makefile guru, I noticed one has to explicitly $(call recurse,checkprep, $(recurse_alldirs_targets)) in `src/test/Makefile` in order for the call to `checkprep` to be made for running make check or installcheck or any other of the variants. Having said all the above, and after spending more time reviewing the whole patch I do think it is worthwhile committing, as long as it is not tripping any users up with unexpected test errors due to incomplete installations. I hope this comment helps a bit more.
Re: Adding a TAP test checking data consistency on standby with minRecoveryPoint
Hi, I applied the patch on current master and run the tests, but I am afraid that the newly introduced test failed on installcheck-world: ```t/016_min_consistency.pl . # Looks like your test exited with 29 before it could output anything. t/016_min_consistency.pl . Dubious, test returned 29 (wstat 7424, 0x1d00) Failed 2/2 subtests Test Summary Report --- t/016_min_consistency.pl (Wstat: 7424 Tests: 0 Failed: 0) Non-zero exit status: 29 Parse errors: Bad plan. You planned 2 tests but ran 0. Files=16, Tests=143, 65 wallclock secs ( 0.04 usr 0.04 sys + 6.53 cusr 5.08 csys = 11.69 CPU) Result: FAIL``` To be honest, I have not checked closely on the failure, still it is the only test failing which by itself should be worthwhile mentioning.
Re: Tighten error control for OpenTransientFile/CloseTransientFile
The following review has been posted through the commitfest application: make installcheck-world: tested, failed Implements feature: not tested Spec compliant: not tested Documentation:not tested The second version of this patch seems to be in order and ready for committer. Thank you for taking the time to code! The new status of this patch is: Ready for Committer
Re: Tighten error control for OpenTransientFile/CloseTransientFile
Overall the patch looks good and according to the previous discussion fulfils its purpose. It might be worthwhile to also check for errors on close in SaveSlotToPath(). pgstat_report_wait_end(); CloseTransientFile(fd); /* rename to permanent file, fsync file and directory */ if (rename(tmppath, path) != 0)
