414 Commits (bba3c35b29d07a27bbf5dd0d7d5e7c7592e02f81)

Author	SHA1	Message	Date
Etsuro Fujita	c2bb02bc2e	Allow asynchronous execution in more cases. ... In commit 27e1f1456, create_append_plan() only allowed the subplan created from a given subpath to be executed asynchronously when it was an async-capable ForeignPath. To extend coverage, this patch handles cases when the given subpath includes some other Path types as well that can be omitted in the plan processing, such as a ProjectionPath directly atop an async-capable ForeignPath, allowing asynchronous execution in partitioned-scan/partitioned-join queries with non-Var tlist expressions and more UNION queries. Andrey Lepikhov and Etsuro Fujita, reviewed by Alexander Pyhalov and Zhihong Yu. Discussion: https://postgr.es/m/659c37a8-3e71-0ff2-394c-f04428c76f08%40postgrespro.ru	3 years ago
Tom Lane	f3dd9fe1dd	Fix postgres_fdw to check shippability of sort clauses properly. ... postgres_fdw would push ORDER BY clauses to the remote side without verifying that the sort operator is safe to ship. Moreover, it failed to print a suitable USING clause if the sort operator isn't default for the sort expression's type. The net result of this is that the remote sort might not have anywhere near the semantics we expect, which'd be disastrous for locally-performed merge joins in particular. We addressed similar issues in the context of ORDER BY within an aggregate function call in commit 7012b132d, but failed to notice that query-level ORDER BY was broken. Thus, much of the necessary logic already existed, but it requires refactoring to be usable in both cases. Back-patch to all supported branches. In HEAD only, remove the core code's copy of find_em_expr_for_rel, which is no longer used and really should never have been pushed into equivclass.c in the first place. Ronan Dunklau, per report from David Rowley; reviews by David Rowley, Ranier Vilela, and myself Discussion: https://postgr.es/m/CAApHDvr4OeC2DBVY--zVP83-K=bYrTD7F8SZDhN4g+pj2f2S-A@mail.gmail.com	4 years ago
Tomas Vondra	db0d67db24	Optimize order of GROUP BY keys ... When evaluating a query with a multi-column GROUP BY clause using sort, the cost may be heavily dependent on the order in which the keys are compared when building the groups. Grouping does not imply any ordering, so we're allowed to compare the keys in arbitrary order, and a Hash Agg leverages this. But for Group Agg, we simply compared keys in the order as specified in the query. This commit explores alternative ordering of the keys, trying to find a cheaper one. In principle, we might generate grouping paths for all permutations of the keys, and leave the rest to the optimizer. But that might get very expensive, so we try to pick only a couple interesting orderings based on both local and global information. When planning the grouping path, we explore statistics (number of distinct values, cost of the comparison function) for the keys and reorder them to minimize comparison costs. Intuitively, it may be better to perform more expensive comparisons (for complex data types etc.) last, because maybe the cheaper comparisons will be enough. Similarly, the higher the cardinality of a key, the lower the probability we’ll need to compare more keys. The patch generates and costs various orderings, picking the cheapest ones. The ordering of group keys may interact with other parts of the query, some of which may not be known while planning the grouping. E.g. there may be an explicit ORDER BY clause, or some other ordering-dependent operation, higher up in the query, and using the same ordering may allow using either incremental sort or even eliminate the sort entirely. The patch generates orderings and picks those minimizing the comparison cost (for various pathkeys), and then adds orderings that might be useful for operations higher up in the plan (ORDER BY, etc.). Finally, it always keeps the ordering specified in the query, on the assumption the user might have additional insights. This introduces a new GUC enable_group_by_reordering, so that the optimization may be disabled if needed. The original patch was proposed by Teodor Sigaev, and later improved and reworked by Dmitry Dolgov. Reviews by a number of people, including me, Andrey Lepikhov, Claudio Freire, Ibrar Ahmed and Zhihong Yu. Author: Dmitry Dolgov, Teodor Sigaev, Tomas Vondra Reviewed-by: Tomas Vondra, Andrey Lepikhov, Claudio Freire, Ibrar Ahmed, Zhihong Yu Discussion: https://postgr.es/m/7c79e6a5-8597-74e8-0671-1c39d124c9d6%40sigaev.ru Discussion: https://postgr.es/m/CA%2Bq6zcW_4o2NC0zutLkOJPsFt80megSpX_dVRo6GK9PC-Jx_Ag%40mail.gmail.com	4 years ago
Etsuro Fujita	5656683503	postgres_fdw: Minor cleanup for pgfdw_abort_cleanup(). ... Commit 85c696112 introduced this function to deduplicate code in the transaction callback functions, but the SQL command passed as an argument to it was useless when it returned before aborting a remote transaction using the command. Modify pgfdw_abort_cleanup() so that it constructs the command when/if necessary, as before, removing the argument from it. Also update comments in pgfdw_abort_cleanup() and one of the calling functions. Etsuro Fujita, reviewed by David Zhang. Discussion: https://postgr.es/m/CAPmGK158hrd%3DZfXmgkmNFHivgh18e4oE2Gz151C2Q4OBDjZ08A%40mail.gmail.com	4 years ago
Michael Paquier	5b81703787	Simplify SRFs using materialize mode in contrib/ modules ... 9e98583 introduced a helper to centralize building their needed state (tuplestore, tuple descriptors, etc.), checking for any errors. This commit updates all places of contrib/ that can be switched to use SetSingleFuncCall() as a drop-in replacement, resulting in the removal of a lot of boilerplate code in all the modules updated by this commit. Per analysis, some places remain as they are: - pg_logdir_ls() in adminpack/ uses historically TYPEFUNC_RECORD as return type, and I suspect that changing it may cause issues at run-time with some of its past versions, down to 1.0. - dblink/ uses a wrapper function doing exactly the work of SetSingleFuncCall(). Here the switch should be possible, but rather invasive so it does not seem the extra backpatch maintenance cost. - tablefunc/, similarly, uses multiple helper functions with portions of SetSingleFuncCall() spread across the code paths of this module. Author: Melanie Plageman Discussion: https://postgr.es/m/CAAKRu_bvDPJoL9mH6eYwvBpPtTGQwbDzfJbCM-OjkSZDu5yTPg@mail.gmail.com	4 years ago
Etsuro Fujita	04e706d423	postgres_fdw: Add support for parallel commit. ... postgres_fdw commits remote (sub)transactions opened on remote server(s) in a local (sub)transaction one by one when the local (sub)transaction commits. This patch allows it to commit the remote (sub)transactions in parallel to improve performance. This is enabled by the server option "parallel_commit". The default is false. Etsuro Fujita, reviewed by Fujii Masao and David Zhang. Discussion: http://postgr.es/m/CAPmGK17dAZCXvwnfpr1eTfknTGdt%3DhYTV9405Gt5SqPOX8K84w%40mail.gmail.com	4 years ago
Tom Lane	88103567cb	Disallow setting bogus GUCs within an extension's reserved namespace. ... Commit 75d22069e tried to throw a warning for setting a custom GUC whose prefix belongs to a previously-loaded extension, if there is no such GUC defined by the extension. But that caused unstable behavior with parallel workers, because workers don't necessarily load extensions and GUCs in the same order their leader did. To make that work safely, we have to completely disallow the case. We now actually remove any such GUCs at the time of initial extension load, and then throw an error not just a warning if you try to add one later. While this might create a compatibility issue for a few people, the improvement in error-detection capability seems worth it; it's hard to believe that there's any good use-case for choosing such GUC names. This also un-reverts 5609cc01c (Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved()), since that function's old name is now even more of a misnomer. Florin Irion and Tom Lane Discussion: https://postgr.es/m/1902182.1640711215@sss.pgh.pa.us	4 years ago
Fujii Masao	94c49d5340	postgres_fdw: Make postgres_fdw.application_name support more escape sequences. ... Commit 6e0cb3dec1 allowed postgres_fdw.application_name to include escape sequences %a (application name), %d (database name), %u (user name) and %p (pid). In addition to them, this commit makes it support the escape sequences for session ID (%c) and cluster name (%C). These are helpful to investigate where each remote transactions came from. Author: Fujii Masao Reviewed-by: Ryohei Takahashi, Kyotaro Horiguchi Discussion: https://postgr.es/m/1041dc9a-c976-049f-9f14-e7d94c29c4b2@oss.nttdata.com	4 years ago
Michael Paquier	d61a361d1a	Remove all traces of tuplestore_donestoring() in the C code ... This routine is a no-op since dd04e95 from 2003, with a macro kept around for compatibility purposes. This has led to the same code patterns being copy-pasted around for no effect, sometimes in confusing ways like in pg_logical_slot_get_changes_guts() from logical.c where the code was actually incorrect. This issue has been discussed on two different threads recently, so rather than living with this legacy, remove any uses of this routine in the C code to simplify things. The compatibility macro is kept to avoid breaking any out-of-core modules that depend on it. Reported-by: Tatsuhito Kasahara, Justin Pryzby Author: Tatsuhito Kasahara Discussion: https://postgr.es/m/20211217200419.GQ17618@telsasoft.com Discussion: https://postgr.es/m/CAP0=ZVJeeYfAeRfmzqAF2Lumdiv4S4FewyBnZd4DPTrsSQKJKw@mail.gmail.com	4 years ago
Etsuro Fujita	9e283fc85d	postgres_fdw: Fix handling of a pending asynchronous request in postgresReScanForeignScan(). ... Commit 27e1f1456 failed to process a pending asynchronous request made for a given ForeignScan node in postgresReScanForeignScan() (if any) in cases where we would only reset the next_tuple counter in that function, contradicting the assumption that there should be no pending asynchronous requests that have been made for async-capable subplans for the parent Append node after ReScan. This led to an assert failure in an assert-enabled build. I think this would also lead to mis-rewinding the cursor in that function in the case where we have already fetched one batch for the ForeignScan node and the asynchronous request has been made for the second batch, because even in that case we would just reset the counter when called from that function, so we would fail to execute MOVE BACKWARD ALL. To fix, modify that function to process the asynchronous request before restarting the scan. While at it, add a comment to a function to match other places. Per bug #17344 from Alexander Lakhin. Back-patch to v14 where the aforesaid commit came in. Patch by me. Test case by Alexander Lakhin, adjusted by me. Reviewed and tested by Alexander Lakhin and Dmitry Dolgov. Discussion: https://postgr.es/m/17344-226b78b00de73a7e@postgresql.org	4 years ago
Etsuro Fujita	6c07f9ebce	postgres_fdw: Fix subabort cleanup of connections used in asynchronous execution. ... Commit 27e1f1456 resets the per-connection states of connections used to scan foreign tables asynchronously during abort cleanup at main transaction end, but it failed to do so during subabort cleanup at subtransaction end, leading to a segmentation fault when re-executing an asynchronous-foreign-table-scan query in a transaction that was cancelled in a subtransaction of it. Fix by modifying pgfdw_abort_cleanup() to reset the per-connection state of a given connection also when called for subabort cleanup. Also, modify that function to do the reset in both the abort-cleanup and subabort-cleanup cases if necessary, to save cycles, and improve a comment on it a little bit. Back-patch to v14 where the aforesaid commit came in. Reviewed by Alexander Pyhalov Discussion: https://postgr.es/m/CAPmGK14cCV-JA7kNsyt2EUTKvZ4xkr2LNRthi1U1C3cqfGppAw@mail.gmail.com	4 years ago
Peter Eisentraut	941460fcf7	Add Boolean node ... Before, SQL-level boolean constants were represented by a string with a cast, and internal Boolean values in DDL commands were usually represented by Integer nodes. This takes the place of both of these uses, making the intent clearer and having some amount of type safety. Reviewed-by: Pavel Stehule <pavel.stehule@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/8c1a2e37-c68d-703c-5a83-7a6077f4f997@enterprisedb.com	4 years ago
Bruce Momjian	27b77ecf9f	Update copyright for 2022 ... Backpatch-through: 10	4 years ago
Fujii Masao	353aa01687	postgres_fdw: Add regression test for postgres_fdw.application_name GUC. ... Commit 449ab63505 added postgres_fdw.application_name GUC that specifies a value for application_name configuration parameter used when postgres_fdw establishes a connection to a foreign server. Also commit 6e0cb3dec1 allowed it to include escape sequences. Both commits added the regression tests for the GUC, but those tests were reverted by commits 98dbef90eb and 5e64ad3697 because they were unstable and caused some buildfarm members to report the failure. This is the third try to add the regression test for postgres_fdw.application_name GUC. One of issues to make the test unstable was to have used postgres_fdw_disconnect_all() to close the existing remote connections. The test expected that the remote connection and its corresponding backend at the remote server disappeared just after postgres_fdw_disconnect_all() was executed, but it could take a bit time for them to disappear. To make sure that they exit, this commit makes the test use pg_terminate_backend() with the timeout at the remote server, instead. If the timeout is set to greater than zero, this function waits until they are actually terminated (or until the given time has passed). Another issue was that the test didn't take into consideration the case where postgres_fdw.application_name containing some escape sequences was converted to the string larger than NAMEDATALEN. In this case it was truncated to less than NAMEDATALEN when it's passed to the remote server, but the test expected wrongly that full string of application_name was always viewable. This commit changes the test so that it can handle that case. Author: Fujii Masao Reviewed-by: Masahiko Sawada, Hayato Kuroda, Kyotaro Horiguchi Discussion: https://postgr.es/m/3220909.1631054766@sss.pgh.pa.us Discussion: https://postgr.es/m/20211224.180006.2247635208768233073.horikyota.ntt@gmail.com Discussion: https://postgr.es/m/e7b61420-a97b-8246-77c4-a0d48fba5a45@oss.nttdata.com	4 years ago
Tom Lane	cab5b9ab2c	Revert changes about warnings/errors for placeholders. ... Revert commits 5609cc01c, 2ed8a8cc5, and 75d22069e until we have a less broken idea of how this should work in parallel workers. Per buildfarm. Discussion: https://postgr.es/m/1640909.1640638123@sss.pgh.pa.us	4 years ago
Tom Lane	5609cc01c6	Rename EmitWarningsOnPlaceholders() to MarkGUCPrefixReserved(). ... This seems like a clearer name for what it does now. Provide a compatibility macro so that extensions don't have to convert to the new name right away. Discussion: https://postgr.es/m/116024.1640111629@sss.pgh.pa.us	4 years ago
Fujii Masao	5e64ad3697	postgres_fdw: Revert unstable tests for postgres_fdw.application_name. ... Commit 6e0cb3dec1 added the tests that check that escape sequences in postgres_fdw.application_name setting are replaced with status information expectedly. But they were unstable and caused some buildfarm members to report the failure. This commit reverts those unstable tests.	4 years ago
Fujii Masao	6e0cb3dec1	postgres_fdw: Allow postgres_fdw.application_name to include escape sequences. ... application_name that used when postgres_fdw establishes a connection to a foreign server can be specified in either or both a connection parameter of a server object and GUC postgres_fdw.application_name. This commit allows those parameters to include escape sequences that begins with % character. Then postgres_fdw replaces those escape sequences with status information. For example, %d and %u are replaced with user name and database name in local server, respectively. This feature enables us to add information more easily to track remote transactions or queries, into application_name of a remote connection. Author: Hayato Kuroda Reviewed-by: Kyotaro Horiguchi, Masahiro Ikeda, Hou Zhijie, Fujii Masao Discussion: https://postgr.es/m/TYAPR01MB5866FAE71C66547C64616584F5EB9@TYAPR01MB5866.jpnprd01.prod.outlook.com Discussion: https://postgr.es/m/TYCPR01MB5870D1E8B949DAF6D3B84E02F5F29@TYCPR01MB5870.jpnprd01.prod.outlook.com	4 years ago
Michael Paquier	2e577c9446	Remove assertion for ALTER TABLE .. DETACH PARTITION CONCURRENTLY ... One code path related to this flavor of ALTER TABLE was checking that the relation to detach has to be a normal table or a partitioned table, which would fail if using the command with a different relation kind. Views, sequences and materialized views cannot be part of a partition tree, so these would cause the command to fail anyway, but the assertion was triggered. Foreign tables can be part of a partition tree, and again the assertion would have failed. The simplest solution is just to remove this assertion, so as we get the same failure as the non-concurrent code path. While on it, add a regression test in postgres_fdw for the concurrent partition detach of a foreign table, as per a suggestion from Alexander Lakhin. Issue introduced in 71f4c8c. Reported-by: Alexander Lakhin Author: Michael Paquier, Alexander Lakhin Reviewed-by: Peter Eisentraut, Kyotaro Horiguchi Discussion: https://postgr.es/m/17339-a9e09aaf38a3457a@postgresql.org Backpatch-through: 14	4 years ago
Tom Lane	1fada5d81e	Add missing EmitWarningsOnPlaceholders() calls. ... Extensions that define any custom GUCs should call EmitWarningsOnPlaceholders after doing so, to help catch misspellings. Many of our contrib modules hadn't gotten the memo on that, though. Also add such calls to src/test/modules extensions that have GUCs. While these aren't really user-facing, they should illustrate good practice not faulty practice. Shinya Kato Discussion: https://postgr.es/m/524fa2c0a34f34b68fbfa90d0760d515@oss.nttdata.com	4 years ago
Fujii Masao	815d61fcd4	postgres_fdw: Report warning when timeout expires while getting query result. ... When aborting remote transaction or sending cancel request to a remote server, postgres_fdw calls pgfdw_get_cleanup_result() to wait for the result of transaction abort query or cancel request to arrive. It fails to get the result if the timeout expires or a connection trouble happens. Previously postgres_fdw reported no warning message even when the timeout expired or a connection trouble happened in pgfdw_get_cleanup_result(). This could make the troubleshooting harder when such an event occurred. This commit makes pgfdw_get_cleanup_result() tell its caller what trouble (timeout or connection error) occurred, on failure, and also makes its caller report the proper warning message based on that information. Author: Fujii Masao Reviewed-by: Bharath Rupireddy Discussion: https://postgr.es/m/15aa988c-722e-ad3e-c936-4420c5b2bfea@oss.nttdata.com	4 years ago
Fujii Masao	557c39bba9	postgres_fdw: Fix unexpected reporting of empty message. ... pgfdw_report_error() in postgres_fdw gets a message from PGresult or PGconn to report an error received from a remote server. Previously if it could get a message from neither of them, it reported empty message unexpectedly. The cause of this issue was that pgfdw_report_error() didn't handle properly the case where no message could be obtained and its local variable message_primary was set to '\0'. This commit improves pgfdw_report_error() so that it reports the message "could not obtain ..." when it gets no message and message_primary is set to '\0'. This is the same behavior as when message_primary is NULL. dblink_res_error() in dblink has the same issue, so this commit also improves it in the same way. Back-patch to all supported branches. Author: Fujii Masao Reviewed-by: Bharath Rupireddy Discussion: https://postgr.es/m/477c16c8-7ea4-20fc-38d5-ed3a77ed616c@oss.nttdata.com	4 years ago
Daniel Gustafsson	ac0db34e0e	Remove PF_USED_FOR_ASSERTS_ONLY from variables in general use ... fsstate in process_pending_requests (in postgres_fdw.c) was added in 8998e3cafa as an assertion-only variable, 1ec7fca859 stated using the variable outside of assertions. rd_index in get_index_column_opclass (in lsyscache.c) was introduced in 2a6368343f, and then promptly used in the fix commit 7e04160390 shortly thereafter. This removes the PG_USED_FOR_ASSERTS_ONLY variable decoration from the above mentioned variables. Reviewed-by: Greg Nancarrow <gregn4422@gmail.com> Discussion: https://postgr.es/m/F959106C-0F21-43A5-B2AE-D007D51ACBEE@yesql.se	4 years ago
Tom Lane	3804539e48	Replace random(), pg_erand48(), etc with a better PRNG API and algorithm. ... Standardize on xoroshiro128** as our basic PRNG algorithm, eliminating a bunch of platform dependencies as well as fundamentally-obsolete PRNG code. In addition, this API replacement will ease replacing the algorithm again in future, should that become necessary. xoroshiro128** is a few percent slower than the drand48 family, but it can produce full-width 64-bit random values not only 48-bit, and it should be much more trustworthy. It's likely to be noticeably faster than the platform's random(), depending on which platform you are thinking about; and we can have non-global state vectors easily, unlike with random(). It is not cryptographically strong, but neither are the functions it replaces. Fabien Coelho, reviewed by Dean Rasheed, Aleksander Alekseev, and myself Discussion: https://postgr.es/m/alpine.DEB.2.22.394.2105241211230.165418@pseudo	4 years ago
David Rowley	e502150f7d	Allow Memoize to operate in binary comparison mode ... Memoize would always use the hash equality operator for the cache key types to determine if the current set of parameters were the same as some previously cached set. Certain types such as floating points where -0.0 and +0.0 differ in their binary representation but are classed as equal by the hash equality operator may cause problems as unless the join uses the same operator it's possible that whichever join operator is being used would be able to distinguish the two values. In which case we may accidentally return in the incorrect rows out of the cache. To fix this here we add a binary mode to Memoize to allow it to the current set of parameters to previously cached values by comparing bit-by-bit rather than logically using the hash equality operator. This binary mode is always used for LATERAL joins and it's used for normal joins when any of the join operators are not hashable. Reported-by: Tom Lane Author: David Rowley Discussion: https://postgr.es/m/3004308.1632952496@sss.pgh.pa.us Backpatch-through: 14, where Memoize was added	4 years ago
Daniel Gustafsson	aa12781b0d	Improve publication error messages ... Commit 81d5995b4b introduced more fine-grained errormessages for incorrect relkinds for publication, while unlogged and temporary tables were reported with using the same message. This provides separate error messages for these types of relpersistence. Author: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Reviewed-by: Peter Eisentraut <peter.eisentraut@enterprisedb.com> Reviewed-by: Jeevan Ladhe <jeevan.ladhe@enterprisedb.com> Reviewed-by: Euler Taveira <euler@eulerto.com> Discussion: https://postgr.es/m/CALj2ACW9S=AswyQHjtO6WMcsergMkCBTtzXGrM8DX26DzfeTLQ@mail.gmail.com	4 years ago
Tom Lane	f8abb0f5e1	postgres_fdw: suppress casts on constants in limited cases. ... When deparsing an expression of the form "remote_var OP constant", we'd normally apply a cast to the constant to make sure that the remote parser thinks it's of the same type we do. However, doing so is often not necessary, and it causes problems if the user has intentionally declared the local column as being of a different type than the remote column. A plausible use-case for that is using text to represent a type that's an enum on the remote side. A comparison on such a column will get shipped as "var = 'foo'::text", which blows up on the remote side because there's no enum = text operator. But if we simply leave off the explicit cast, the comparison will do exactly what the user wants. It's possible to do this without major risk of semantic problems, by relying on the longstanding parser heuristic that "if one operand of an operator is of type unknown, while the other one has a known type, assume that the unknown operand is also of that type". Hence, this patch leaves off the cast only if (a) the operator inputs have the same type locally; (b) the constant will print as a string literal or NULL, both of which are initially taken as type unknown; and (c) the non-Const input is a plain foreign Var. Rule (c) guarantees that the remote parser will know the type of the non-Const input; moreover, it means that if this cast-omission does cause any semantic surprises, that can only happen in cases where the local column has a different type than the remote column. That wasn't guaranteed to work anyway, and this patch should represent a net usability gain for such cases. One point that I (tgl) remain slightly uncomfortable with is that we will ignore an implicit RelabelType when deciding if the non-Const input is a plain Var. That makes it a little squishy to argue that the remote should resolve the Const as being of the same type as its Var, because then our Const is not the same type as our Var. However, if we don't do that, then this hack won't work as desired if the user chooses to use varchar rather than text to represent some remote column. That seems useful, so do it like this for now. We might have to give up the RelabelType-ignoring bit if any problems surface. Dian Fay, with review and kibitzing by me Discussion: https://postgr.es/m/C9LU294V7K4F.34LRRDU449O45@lamia	4 years ago
Fujii Masao	5fedf7417b	Improve HINT message that FDW reports when there are no valid options. ... The foreign data wrapper's validator function provides a HINT message with list of valid options for the object specified in CREATE or ALTER command, when the option given in the command is invalid. Previously postgresql_fdw_validator() and the validator functions for postgres_fdw and dblink_fdw worked in that way even there were no valid options in the object, which could lead to the HINT message with empty list (because there were no valid options). For example, ALTER FOREIGN DATA WRAPPER postgres_fdw OPTIONS (format 'csv') reported the following ERROR and HINT messages. This behavior was confusing. ERROR: invalid option "format" HINT: Valid options in this context are: There is no such issue in file_fdw. The validator function for file_fdw reports the HINT message "There are no valid options in this context." instead in that case. This commit improves postgresql_fdw_validator() and the validator functions for postgres_fdw and dblink_fdw so that they do likewise. For example, this change causes the above ALTER FOREIGN DATA WRAPPER command to report the following messages. ERROR: invalid option "nonexistent" HINT: There are no valid options in this context. Author: Kosei Masumura Reviewed-by: Bharath Rupireddy, Fujii Masao Discussion: https://postgr.es/m/557d06cebe19081bfcc83ee2affc98d3@oss.nttdata.com	4 years ago
Etsuro Fujita	8c7be86883	postgres_fdw: Move comments about elog level in (sub)abort cleanup. ... The comments were misplaced when adding postgres_fdw. Fix that by moving the comments to more appropriate functions. Author: Etsuro Fujita Backpatch-through: 9.6 Discussion: https://postgr.es/m/CAPmGK164sAXQtC46mDFyu6d-T25Mzvh5qaRNkit06VMmecYnOA%40mail.gmail.com	4 years ago
Etsuro Fujita	972c7c6567	postgres_fdw: Fix comments in connection.c. ... Commit 27e1f1456 missed updating some comments. Reviewed-by: Bharath Rupireddy Backpatch-through: 14 Discussion: https://postgr.es/m/CAPmGK15Q2Nm6U%2Ba_GwskrWFEVBZ9_3VKOvRrprGufpx91M_3Sw%40mail.gmail.com	4 years ago
Tom Lane	3071bbfe44	Fix null-pointer crash in postgres_fdw's conversion_error_callback. ... Commit c7b7311f6 adjusted conversion_error_callback to always use information from the query's rangetable, to avoid doing catalog lookups in an already-failed transaction. However, as a result of the utterly inadequate documentation for make_tuple_from_result_row, I failed to realize that fsstate could be NULL in some contexts. That led to a crash if we got a conversion error in such a context. Fix by falling back to the previous coding when fsstate is NULL. Improve the commentary, too. Per report from Andrey Borodin. Back-patch to 9.6, like the previous patch. Discussion: https://postgr.es/m/08916396-55E4-4D68-AB3A-BD6066F9E5C0@yandex-team.ru	4 years ago
Fujii Masao	85c6961128	postgres_fdw: Refactor transaction rollback code to avoid code duplication. ... In postgres_fdw, pgfdw_xact_callback() and pgfdw_subxact_callback() callback functions do almost the same thing to rollback remote toplevel- and sub-transaction. But previously their such rollback logics were implemented separately in each function and in different way. Which could decrease the readability and maintainability of the code. To fix the issue, this commit creates the common function to rollback remote transactions, and makes those callback functions use it. Which allows us to avoid unnecessary code duplication. Author: Fujii Masao Reviewed-by: Zhihong Yu, Bharath Rupireddy Discussion: https://postgr.es/m/62fbb63a-d46c-fb47-a56d-f6be1909aa44@oss.nttdata.com	4 years ago
Noah Misch	b073c3ccd0	Revoke PUBLIC CREATE from public schema, now owned by pg_database_owner. ... This switches the default ACL to what the documentation has recommended since CVE-2018-1058. Upgrades will carry forward any old ownership and ACL. Sites that declined the 2018 recommendation should take a fresh look. Recipes for commissioning a new database cluster from scratch may need to create a schema, grant more privileges, etc. Out-of-tree test suites may require such updates. Reviewed by Peter Eisentraut. Discussion: https://postgr.es/m/20201031163518.GB4039133@rfd.leadboat.com	4 years ago
Peter Eisentraut	639a86e36a	Remove Value node struct ... The Value node struct is a weird construct. It is its own node type, but most of the time, it actually has a node type of Integer, Float, String, or BitString. As a consequence, the struct name and the node type don't match most of the time, and so it has to be treated specially a lot. There doesn't seem to be any value in the special construct. There is very little code that wants to accept all Value variants but nothing else (and even if it did, this doesn't provide any convenient way to check it), and most code wants either just one particular node type (usually String), or it accepts a broader set of node types besides just Value. This change removes the Value struct and node type and replaces them by separate Integer, Float, String, and BitString node types that are proper node types and structs of their own and behave mostly like normal node types. Also, this removes the T_Null node tag, which was previously also a possible variant of Value but wasn't actually used outside of the Value contained in A_Const. Replace that by an isnull field in A_Const. Reviewed-by: Dagfinn Ilmari Mannsåker <ilmari@ilmari.org> Reviewed-by: Kyotaro Horiguchi <horikyota.ntt@gmail.com> Discussion: https://www.postgresql.org/message-id/flat/5ba6bc5b-3f95-04f2-2419-f8ddb4c046fb@enterprisedb.com	4 years ago
Fujii Masao	98dbef90eb	postgres_fdw: Revert unstable tests for postgres_fdw.application_name. ... Commit 449ab63505 added the tests that check that postgres_fdw.application_name GUC works as expected. But they were unstable and caused some buildfarm members to report the failure. This commit reverts those unstable tests. Reported-by: Tom Lane as per buildfarm Discussion: https://postgr.es/m/3220909.1631054766@sss.pgh.pa.us	4 years ago
Fujii Masao	449ab63505	postgres_fdw: Allow application_name of remote connection to be set via GUC. ... This commit adds postgres_fdw.application_name GUC which specifies a value for application_name configuration parameter used when postgres_fdw establishes a connection to a foreign server. This GUC setting always overrides application_name option of the foreign server object. This GUC is useful when we want to specify our own application_name per remote connection. Previously application_name of a remote connection could be set basically only via options of a server object. But which meant that every session connecting to the same foreign server basically should use the same application_name. Also if we want to change the setting, we had to execute "ALTER SERVER ... OPTIONS ..." command. It was inconvenient. Author: Hayato Kuroda Reviewed-by: Masahiro Ikeda, Fujii Masao Discussion: https://postgr.es/m/TYCPR01MB5870D1E8B949DAF6D3B84E02F5F29@TYCPR01MB5870.jpnprd01.prod.outlook.com	4 years ago
Tom Lane	2dc53fe2a7	Refactor postgresImportForeignSchema to avoid code duplication. ... Avoid repeating fragments of the query we're building, along the same lines as recent cleanup in pg_dump. I got annoyed about this because aa769f80e broke my pending patch to change postgres_fdw's collation handling, due to each of us having incompletely done this same refactoring. Let's finish that job in hopes of having a more stable base.	4 years ago
Etsuro Fujita	aa769f80ed	postgres_fdw: Fix issues with generated columns in foreign tables. ... postgres_fdw imported generated columns from the remote tables as plain columns, and caused failures like "ERROR: cannot insert a non-DEFAULT value into column "foo"" when inserting into the foreign tables, as it tried to insert values into the generated columns. To fix, we do the following under the assumption that generated columns in a postgres_fdw foreign table are defined so that they represent generated columns in the underlying remote table: * Send DEFAULT for the generated columns to the foreign server on insert or update, not generated column values computed on the local server. * Add to postgresImportForeignSchema() an option "import_generated" to include column generated expressions in the definitions of foreign tables imported from a foreign server. The option is true by default. The assumption seems reasonable, because that would make a query of the postgres_fdw foreign table return values for the generated columns that are consistent with the generated expression. While here, fix another issue in postgresImportForeignSchema(): it tried to include column generated expressions as column default expressions in the foreign table definitions when the import_default option was enabled. Per bug #16631 from Daniel Cherniy. Back-patch to v12 where generated columns were added. Discussion: https://postgr.es/m/16631-e929fe9db0ffc7cf%40postgresql.org	4 years ago
Etsuro Fujita	a8ed9bd59d	Fix oversight in commit 1ec7fca859. ... I failed to account for the possibility that when ExecAppendAsyncEventWait() notifies multiple async-capable nodes using postgres_fdw, a preceding node might invoke process_pending_request() to process a pending asynchronous request made by a succeeding node. In that case the succeeding node should produce a tuple to return to the parent Append node from tuples fetched by process_pending_request() when notified. Repair. Per buildfarm via Michael Paquier. Back-patch to v14, like the previous commit. Thanks to Tom Lane for testing. Discussion: https://postgr.es/m/YQP0UPT8KmPiHTMs%40paquier.xyz	4 years ago
Tom Lane	5d44fff01e	In postgres_fdw, allow CASE expressions to be pushed to the remote server. ... This is simple enough except for the need to check whether CaseTestExpr nodes have a collation that is not derived from a remote Var. For that, examine the CASE's "arg" expression and then pass that info down into the recursive examination of the WHEN expressions. Alexander Pyhalov, reviewed by Gilles Darold and myself Discussion: https://postgr.es/m/fda09032e90d85d9b726a41e03f9097f@postgrespro.ru	4 years ago
Etsuro Fujita	1ec7fca859	postgres_fdw: Fix handling of pending asynchronous requests. ... A pending asynchronous request is handled by process_pending_request(), which previously not only processed an in-progress remote query but performed ExecForeignScan() to produce a tuple to return to the local server asynchronously from the result of the remote query. But that led to a server crash when executing a query or led to an "InstrStartNode called twice in a row" or "InstrEndLoop called on running node" failure when doing EXPLAIN ANALYZE of it, in cases where the plan tree for it contained multiple async-capable nodes accessing the same initplan/subplan that contained multiple async-capable nodes scanning the same foreign tables as for the parent async-capable nodes, as reported by Andrey Lepikhov. The reason is that the second step in process_pending_request() invoked when executing the initplan/subplan for one of the parent async-capable nodes caused recursive execution of the initplan/subplan for another of the parent async-capable nodes. To fix, split process_pending_request() into the two steps and postpone the second step until ForeignAsyncConfigureWait() is called for each of the pending asynchronous requests. Also, in ExecAppendAsyncEventWait() we assumed that FDWs would register at least one wait event in a WaitEventSet created there when they were called from ForeignAsyncConfigureWait() in that function, but allow FDWs to register zero wait events in the WaitEventSet; modify ExecAppendAsyncEventWait() to just return in that case. Oversight in commit 27e1f1456. Back-patch to v14 where that commit went in. Andrey Lepikhov and Etsuro Fujita Discussion: https://postgr.es/m/fe5eaa19-1704-e4a4-76ee-3b9d37ade399@postgrespro.ru	4 years ago
Fujii Masao	0e1275fb07	Avoid using ambiguous word "non-negative" in error messages. ... The error messages using the word "non-negative" are confusing because it's ambiguous about whether it accepts zero or not. This commit improves those error messages by replacing it with less ambiguous word like "greater than zero" or "greater than or equal to zero". Also this commit added the note about the word "non-negative" to the error message style guide, to help writing the new error messages. When postgres_fdw option fetch_size was set to zero, previously the error message "fetch_size requires a non-negative integer value" was reported. This error message was outright buggy. Therefore back-patch to all supported versions where such buggy error message could be thrown. Reported-by: Hou Zhijie Author: Bharath Rupireddy Reviewed-by: Kyotaro Horiguchi, Fujii Masao Discussion: https://postgr.es/m/OS0PR01MB5716415335A06B489F1B3A8194569@OS0PR01MB5716.jpnprd01.prod.outlook.com	4 years ago
Tom Lane	48c5c90682	Use the "pg_temp" schema alias in EXPLAIN and related output. ... This patch causes EXPLAIN output to refer to objects that are in the current session's temp schema with the "pg_temp" schema alias rather than that schema's actual name. This is useful for our own testing purposes since it will stabilize EXPLAIN VERBOSE output for such cases, allowing us to use that in regression tests. It should be less confusing for end users too. Since ruleutils.c needs to change behavior for this, the change also leaks into a few other users of ruleutils.c, for example pg_get_viewdef(). AFAICS that won't cause any problems. We did find that aggressively trying to change this behavior across-the-board would cause issues, but as long as "pg_temp" only appears within generated SQL text, I think it'll be fine. Along the way, make get_namespace_name_or_temp conform to the same API as get_namespace_name, ie that it returns a palloc'd string or NULL. The current behavior hasn't caused any bugs since no callers attempt to pfree the result, but if it gets more widespread usage that could become a problem. Amul Sul, reviewed and extended by me Discussion: https://postgr.es/m/CAAJ_b97W=QaGmag9AhWNbmx3uEYsNkXWL+OVW1_E1D3BtgWvtw@mail.gmail.com	4 years ago
David Rowley	83f4fcc655	Change the name of the Result Cache node to Memoize ... "Result Cache" was never a great name for this node, but nobody managed to come up with another name that anyone liked enough. That was until David Johnston mentioned "Node Memoization", which Tom Lane revised to just "Memoize". People seem to like "Memoize", so let's do the rename. Reviewed-by: Justin Pryzby Discussion: https://postgr.es/m/20210708165145.GG1176@momjian.us Backpatch-through: 14, where Result Cache was introduced	4 years ago
Tom Lane	d68a003912	Rename debug_invalidate_system_caches_always to debug_discard_caches. ... The name introduced by commit 4656e3d66 was agreed to be unreasonably long. To match this change, rename initdb's recently-added --clobber-cache option to --discard-caches. Discussion: https://postgr.es/m/1374320.1625430433@sss.pgh.pa.us	4 years ago
Tom Lane	b9734c13f1	Fix crash in postgres_fdw for provably-empty remote UPDATE/DELETE. ... In 86dc90056, I'd written find_modifytable_subplan with the assumption that if the immediate child of a ModifyTable is a Result, it must be a projecting Result with a subplan. However, if the UPDATE or DELETE has a provably-constant-false WHERE clause, that's not so: we'll generate a dummy subplan with a childless Result. Add the missing null-check so we don't crash on such cases. Per report from Alexander Pyhalov. Discussion: https://postgr.es/m/b9a6f53549456b2f3e2fd150dcd79d72@postgrespro.ru	4 years ago
Fujii Masao	d854720df6	postgres_fdw: Tighten up allowed values for batch_size, fetch_size options. ... Previously the values such as '100$%$#$#', '9,223,372,' were accepted and treated as valid integers for postgres_fdw options batch_size and fetch_size. Whereas this is not the case with fdw_startup_cost and fdw_tuple_cost options for which an error is thrown. This was because endptr was not used while converting strings to integers using strtol. This commit changes the logic so that it uses parse_int function instead of strtol as it serves the purpose by returning false in case if it is unable to convert the string to integer. Note that this function also rounds off the values such as '100.456' to 100 and '100.567' or '100.678' to 101. While on this, use parse_real for fdw_startup_cost and fdw_tuple_cost options. Since parse_int and parse_real are being used for reloptions and GUCs, it is more appropriate to use in postgres_fdw rather than using strtol and strtod directly. Back-patch to v14. Author: Bharath Rupireddy Reviewed-by: Ashutosh Bapat, Tom Lane, Kyotaro Horiguchi, Fujii Masao Discussion: https://postgr.es/m/CALj2ACVMO6wY5Pc4oe1OCgUOAtdjHuFsBDw8R5uoYR86eWFQDA@mail.gmail.com	4 years ago
Tom Lane	c7b7311f61	Avoid doing catalog lookups in postgres_fdw's conversion_error_callback. ... As in 50371df26, this is a bad idea since the callback can't really know what error is being thrown and thus whether or not it is safe to attempt catalog accesses. Rather than pushing said accesses into the mainline code where they'd usually be a waste of cycles, we can look at the query's rangetable instead. This change does mean that we'll be printing query aliases (if any were used) rather than the table or column's true name. But that doesn't seem like a bad thing: it's certainly a more useful definition in self-join cases, for instance. In any case, it seems unlikely that any applications would be depending on this detail, so it seems safe to change. Patch by me. Original complaint by Andres Freund; Bharath Rupireddy noted the connection to conversion_error_callback. Discussion: https://postgr.es/m/20210106020229.ne5xnuu6wlondjpe@alap3.anarazel.de	4 years ago
Tom Lane	8021770909	Further stabilize postgres_fdw test. ... The queries involving ft1_nopw don't stably return the same row anymore. I surmise that an autovacuum hitting "S 1"."T 1" right after the updates introduced by f61db909d/5843659d0 freed some space, changing where subsequent insertions get stored. It's only by good luck that these results were stable before, though, since a LIMIT without ORDER BY isn't well defined, and it's not like we've ever treated that table as append-only in this test script. Since we only really care whether these commands succeed or not, just replace "SELECT *" with "SELECT 1". Report: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=crake&dt=2021-06-23%2019%3A52%3A08	4 years ago
Tom Lane	5843659d09	Stabilize test case added by commit f61db909d. ... Buildfarm members ayu and tern have sometimes shown a different plan than expected for this query. I'd been unable to reproduce that before today, but I finally realized what is happening. If there is a concurrent open transaction (probably an autovacuum run in the buildfarm, but this can also be arranged manually), then the index entries for the rows removed by the DELETE a few lines up are not killed promptly, causing a change in the planner's estimate of the extremal value of ft2.c1, which moves the rowcount estimate for "c1 > 1100" by enough to change the join plan from nestloop to hash. To fix, change the query condition to "c1 > 1000", causing the hash plan to be preferred whether or not a concurrent open transaction exists. Since this UPDATE is tailored to be a no-op, nothing else changes. Report: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=ayu&dt=2021-06-09%2022%3A45%3A48 Report: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=ayu&dt=2021-06-13%2022%3A38%3A18 Report: https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=tern&dt=2021-06-20%2004%3A55%3A36	4 years ago