185 Commits (d642d23064ea1bf29137336e2a17d1bba988cb41)

Author	SHA1	Message	Date
Etsuro Fujita	d642d23064	Disallow collecting transition tuples from child foreign tables. ... Commit 9e6104c66 disallowed transition tables on foreign tables, but failed to account for cases where a foreign table is a child table of a partitioned/inherited table on which transition tables exist, leading to incorrect transition tuples collected from such foreign tables for queries on the parent table triggering transition capture. This occurred not only for inherited UPDATE/DELETE but for partitioned INSERT later supported by commit 3d956d956, which should have handled it at least for the INSERT case, but didn't. To fix, modify ExecAR*Triggers to throw an error if the given relation is a foreign table requesting transition capture. Also, this commit fixes make_modifytable so that in case of an inherited UPDATE/DELETE triggering transition capture, FDWs choose normal operations to modify child foreign tables, not DirectModify; which is needed because they would otherwise skip the calls to ExecAR*Triggers at execution, causing unexpected behavior. Author: Etsuro Fujita <etsuro.fujita@gmail.com> Reviewed-by: Amit Langote <amitlangote09@gmail.com> Discussion: https://postgr.es/m/CAPmGK14QJYikKzBDCe3jMbpGENnQ7popFmbEgm-XTNuk55oyHg%40mail.gmail.com Backpatch-through: 13	1 month ago
Masahiko Sawada	e81e53a0c1	Restrict accesses to non-system views and foreign tables during pg_dump. ... When pg_dump retrieves the list of database objects and performs the data dump, there was possibility that objects are replaced with others of the same name, such as views, and access them. This vulnerability could result in code execution with superuser privileges during the pg_dump process. This issue can arise when dumping data of sequences, foreign tables (only 13 or later), or tables registered with a WHERE clause in the extension configuration table. To address this, pg_dump now utilizes the newly introduced restrict_nonsystem_relation_kind GUC parameter to restrict the accesses to non-system views and foreign tables during the dump process. This new GUC parameter is added to back branches too, but these changes do not require cluster recreation. Back-patch to all supported branches. Reviewed-by: Noah Misch Security: CVE-2024-7348 Backpatch-through: 12	1 year ago
Etsuro Fujita	f39f3e0fbe	postgres_fdw: Avoid "cursor can only scan forward" error. ... Commit d844cd75a disallowed rewind in a non-scrollable cursor to resolve anomalies arising from such a cursor operation. However, this failed to take into account the assumption in postgres_fdw that when rescanning a foreign relation, it can rewind the cursor created for scanning the foreign relation without specifying the SCROLL option, regardless of its scrollability, causing this error when it tried to do such a rewind in a non-scrollable cursor. Fix by modifying postgres_fdw to instead recreate the cursor, regardless of its scrollability, when rescanning the foreign relation. (If we had a way to check its scrollability, we could improve this by rewinding it if it is scrollable and recreating it if not, but we do not have it, so this commit modifies it to recreate it in any case.) Per bug #17889 from Eric Cyr. Devrim Gunduz also reported this problem. Back-patch to v15 where that commit enforced the prohibition. Reviewed by Tom Lane. Discussion: https://postgr.es/m/17889-e8c39a251d258dda%40postgresql.org Discussion: https://postgr.es/m/b415ac3255f8352d1ea921cf3b7ba39e0587768a.camel%40gunduz.org	1 year ago
Etsuro Fujita	b33c141cc5	postgres_fdw: Refuse to send FETCH FIRST WITH TIES to remote servers. ... Previously, when considering LIMIT pushdown, postgres_fdw failed to check whether the query has this clause, which led to pushing false LIMIT clauses, causing incorrect results. This clause has been supported since v13, so we need to do a remote-version check before deciding that it will be safe to push such a clause, but we do not currently have a way to do the check (without accessing the remote server); disable pushing such a clause for now. Oversight in commit 357889eb1. Back-patch to v13, where that commit added the support. Per bug #18467 from Onder Kalaci. Patch by Japin Li, per a suggestion from Tom Lane, with some changes to the comments by me. Review by Onder Kalaci, Alvaro Herrera, and me. Discussion: https://postgr.es/m/18467-7bb89084ff03a08d%40postgresql.org	1 year ago
David Rowley	ab64b275ad	Fix deparsing of Consts in postgres_fdw ORDER BY ... For UNION ALL queries where a union child query contained a foreign table, if the targetlist of that query contained a constant, and the top-level query performed an ORDER BY which contained the column for the constant value, then postgres_fdw would find the EquivalenceMember with the Const and then try to produce an ORDER BY containing that Const. This caused problems with INT typed Consts as these could appear to be requests to order by an ordinal column position rather than the constant value. This could lead to either an error such as: ERROR: ORDER BY position <int const> is not in select list or worse, if the constant value is a valid column, then we could just sort by the wrong column altogether. Here we fix this issue by just not including these Consts in the ORDER BY clause. In passing, add a new section for testing ORDER BY in the postgres_fdw tests and move two existing tests which were misplaced in the WHERE clause testing section into it. Reported-by: Michał Kłeczek Reviewed-by: Ashutosh Bapat, Richard Guo Bug: #18381 Discussion: https://postgr.es/m/0714C8B8-8D82-4ABB-9F8D-A0C3657E7B6E%40kleczek.org Discussion: https://postgr.es/m/18381-137456acd168bf93%40postgresql.org Backpatch-through: 12, oldest supported version	2 years ago
Heikki Linnakangas	481d7d1c01	Fix resource leak when a FDW's ForeignAsyncRequest function fails ... If an error is thrown after calling CreateWaitEventSet(), the memory of a WaitEventSet is free'd as it's allocated in the short-lived memory context, but the file descriptor (on epoll- or kqueue-based systems) or handles (on Windows) that it contains are leaked. Use PG_TRY-FINALLY to ensure it gets freed. (On master, I will apply a better fix, using ResourceOwners to track the WaitEventSet, but that's not backpatchable.) The added test doesn't check for leaking resources, so it passed even before this commit. But at least it covers the code path. In the passing, fix misleading comment on what the 'nevents' argument to WaitEventSetWait means. Report by Alexander Lakhin, analysis and suggestion for the fix by Tom Lane. Fixes bug #17828. Backpatch to v14 where async execution was introduced, but master gets a different fix. Discussion: https://www.postgresql.org/message-id/17828-122da8cba23236be@postgresql.org Discussion: https://www.postgresql.org/message-id/472235.1678387869@sss.pgh.pa.us	2 years ago
Etsuro Fujita	3daee7fb08	postgres_fdw: Fix test for parameterized foreign scan. ... Commit e4106b252 should have updated this test, but did not; back-patch to all supported branches. Reviewed by Richard Guo. Discussion: http://postgr.es/m/CAPmGK15nR0NXLSCKQAcqbZbTzrzd5MozowWnTnGfPkayndF43Q%40mail.gmail.com	2 years ago
Etsuro Fujita	d1ef5631e6	Disallow replacing joins with scans in problematic cases. ... Commit e7cb7ee14, which introduced the infrastructure for FDWs and custom scan providers to replace joins with scans, failed to add support handling of pseudoconstant quals assigned to replaced joins in createplan.c, leading to an incorrect plan without a gating Result node when postgres_fdw replaced a join with such a qual. To fix, we could add the support by 1) modifying the ForeignPath and CustomPath structs to store the list of RestrictInfo nodes to apply to the join, as in JoinPaths, if they represent foreign and custom scans replacing a join with a scan, and by 2) modifying create_scan_plan() in createplan.c to use that list in that case, instead of the baserestrictinfo list, to get pseudoconstant quals assigned to the join; but #1 would cause an ABI break. So fix by modifying the infrastructure to just disallow replacing joins with such quals. Back-patch to all supported branches. Reported by Nishant Sharma. Patch by me, reviewed by Nishant Sharma and Richard Guo. Discussion: https://postgr.es/m/CADrsxdbcN1vejBaf8a%2BQhrZY5PXL-04mCd4GDu6qm6FigDZd6Q%40mail.gmail.com	2 years ago
Tomas Vondra	23ce74811a	Remove expensive test of postgres_fdw batch inserts ... The test inserted 70k rows into a foreign table, in order to verify correct behavior with more than 65535 parameters, and was added in response to a bug report. However, this is rather expensive, especially when running the tests under valgrind, CLOBBER_CACHE_ALWAYS etc. It doesn't seem worth it to keep running the test, so remove it from all branches (14+). Backpatch-through: 14 Discussion: https://postgr.es/m/2131017.1623451468@sss.pgh.pa.us	2 years ago
Michael Paquier	aa6177c882	Fix buffer refcount leak with FDW bulk inserts ... The leak would show up when using batch inserts with foreign tables included in a partition tree, as the slots used in the batch were not reset once processed. In order to fix this problem, some ExecClearTuple() are added to clean up the slots used once a batch is filled and processed, mapping with the number of slots currently in use as tracked by the counter ri_NumSlots. This buffer refcount leak has been introduced in b676ac4 with the addition of the executor facility to improve bulk inserts for FDWs, so backpatch down to 14. Alexander has provided the patch (slightly modified by me). The test for postgres_fdw comes from me, based on the test case that the author has sent in the report. Author: Alexander Pyhalov Discussion: https://postgr.es/m/b035780a740efd38dc30790c76927255@postgrespro.ru Backpatch-through: 14	2 years ago
Tom Lane	b15db7f690	Drop test view when done with it. ... The view just added by commit 53fe7e6cb decompiles differently in v15 than HEAD (presumably as a consequence of 47bb9db75). That causes failures in cross-version upgrade testing. We could teach AdjustUpgrade.pm to compensate for that, but it seems less painful to just drop the view after we're done with it. Per buildfarm.	3 years ago
Tom Lane	bc77be7145	Harden postgres_fdw tests against unexpected cache flushes. ... postgres_fdw will close its remote session if an sinval cache reset occurs, since it's possible that that means some FDW parameters changed. We had two tests that were trying to ensure that the session remains alive by setting debug_discard_caches = 0; but that's not sufficient. Even though the tests seem stable enough in the buildfarm, they flap a lot under CI. In the first test, which is checking the ability to recover from a lost connection, we can stabilize the results by just not caring whether pg_terminate_backend() finds a victim backend. If a reset did happen, there won't be a session to terminate anymore, but the test can proceed anyway. (Arguably, we are then not testing the unintentional-disconnect case, but as long as that scenario is exercised in most runs I think it's fine; testing the reset-driven case is of value too.) In the second test, which is trying to verify the application_name displayed in pg_stat_activity by a remote session, we had a race condition in that the remote session might go away before we can fetch its pg_stat_activity entry. We can close that race and make the test more certainly test what it intends to by arranging things so that the remote session itself fetches its pg_stat_activity entry (based on PID rather than a somewhat-circular assumption about the application name). Both tests now demonstrably pass under debug_discard_caches = 1, so we can remove that hack. Back-patch into relevant back branches. Discussion: https://postgr.es/m/20230226194340.u44bkfgyz64c67i6@awork3.anarazel.de	3 years ago
Etsuro Fujita	fc02019c09	Fix handling of pending inserts in nodeModifyTable.c. ... Commit b663a4136, which allowed FDWs to INSERT rows in bulk, added to nodeModifyTable.c code to flush pending inserts to the foreign-table result relation(s) before completing processing of the ModifyTable node, but the code failed to take into account the case where the INSERT query has modifying CTEs, leading to incorrect results. Also, that commit failed to flush pending inserts before firing BEFORE ROW triggers so that rows are visible to such triggers. In that commit we scanned through EState's es_tuple_routing_result_relations or es_opened_result_relations list to find the foreign-table result relations to which pending inserts are flushed, but that would be inefficient in some cases. So to fix, 1) add a List member to EState to record the insert-pending result relations, and 2) modify nodeModifyTable.c so that it adds the foreign-table result relation to the list in ExecInsert() if appropriate, and flushes pending inserts properly using the list where needed. While here, fix a copy-and-pasteo in a comment in ExecBatchInsert(), which was added by that commit. Back-patch to v14 where that commit appeared. Discussion: https://postgr.es/m/CAPmGK16qutyCmyJJzgQOhfBq%3DNoGDqTB6O0QBZTihrbqre%2BoxA%40mail.gmail.com	3 years ago
Alvaro Herrera	16d11d6843	Disallow MERGE cleanly for foreign partitions ... While directly targetting a foreign table with MERGE was already expressly forbidden, we failed to catch the case of a partitioned table that has a foreign table as a partition; and the result if you try is an incomprehensible error. Fix that by adding a specific check. Backpatch to 15. Reported-by: Tatsuhiro Nakamori <bt22nakamorit@oss.nttdata.com> Discussion: https://postgr.es/m/bt22nakamorit@oss.nttdata.com	3 years ago
Etsuro Fujita	64387c5408	postgres_fdw: Avoid 'variable not found in subplan target list' error. ... The tlist of the EvalPlanQual outer plan for a ForeignScan node is adjusted to produce a tuple whose descriptor matches the scan tuple slot for the ForeignScan node. But in the case where the outer plan contains an extra Sort node, if the new tlist contained columns required only for evaluating PlaceHolderVars or columns required only for evaluating local conditions, this would cause setrefs.c to fail with the error. The cause of this is that when creating the outer plan by injecting the Sort node into an alternative local join plan that could emit such extra columns as well, we fail to arrange for the outer plan to propagate them up through the Sort node, causing setrefs.c to fail to match up them in the new tlist to what is available from the outer plan. Repair. Per report from Alexander Pyhalov. Richard Guo and Etsuro Fujita, reviewed by Alexander Pyhalov and Tom Lane. Backpatch to all supported versions. Discussion: http://postgr.es/m/cfb17bf6dfdf876467bd5ef533852d18%40postgrespro.ru	3 years ago
Etsuro Fujita	1d49db2598	postgres_fdw: Disable batch insertion when there are WCO constraints. ... When inserting a view referencing a foreign table that has WITH CHECK OPTION constraints, in single-insert mode postgres_fdw retrieves the data that was actually inserted on the remote side so that the WITH CHECK OPTION constraints are enforced with the data locally, but in batch-insert mode it cannot currently retrieve the data (except for the row first inserted through the view), resulting in enforcing the WITH CHECK OPTION constraints with the data passed from the core (except for the first-inserted row), which led to incorrect results when inserting into a view referencing a foreign table in which a remote BEFORE ROW INSERT trigger changes the rows inserted through the view so that they violate the view's WITH CHECK OPTION constraint. Also, the query inserting into the view caused an assertion failure in assert-enabled builds. Fix these by disabling batch insertion when inserting into such a view. Back-patch to v14 where batch insertion was added. Discussion: https://postgr.es/m/CAPmGK17LpbTZs4m4a_6THP54UBeK9fHvX8aVVA%2BC6yEZDZwQcg%40mail.gmail.com	3 years ago
Tom Lane	b8032f4816	postgres_fdw: set search_path to 'pg_catalog' while deparsing constants. ... The motivation for this is to ensure successful transmission of the values of constants of regconfig and other reg* types. The remote will be reading them with search_path = 'pg_catalog', so schema qualification is necessary when referencing objects in other schemas. Per bug #17483 from Emmanuel Quincerot. Back-patch to all supported versions. (There's some other stuff to do here, but it's less back-patchable.) Discussion: https://postgr.es/m/1423433.1652722406@sss.pgh.pa.us	3 years ago
Etsuro Fujita	5c854e7a2c	Disable asynchronous execution if using gating Result nodes. ... mark_async_capable_plan(), which is called from create_append_plan() to determine whether subplans are async-capable, failed to take into account that the given subplan created from a given subpath might include a gating Result node if the subpath is a SubqueryScanPath or ForeignPath, causing a segmentation fault there when the subplan created from a SubqueryScanPath includes the Result node, or causing ExecAsyncRequest() to throw an error about an unrecognized node type when the subplan created from a ForeignPath includes the Result node, because in the latter case the Result node was unintentionally considered as async-capable, but we don't currently support executing Result nodes asynchronously. Fix by modifying mark_async_capable_plan() to disable asynchronous execution in such cases. Also, adjust code in the ProjectionPath case in mark_async_capable_plan(), for consistency with other cases, and adjust/improve comments there. is_async_capable_path() added in commit 27e1f1456, which was rewritten to mark_async_capable_plan() in a later commit, has the same issue, causing the error at execution mentioned above, so back-patch to v14 where the aforesaid commit went in. Per report from Justin Pryzby. Etsuro Fujita, reviewed by Zhihong Yu and Justin Pryzby. Discussion: https://postgr.es/m/20220408124338.GK24419%40telsasoft.com	3 years ago
Etsuro Fujita	4eea2202be	postgres_fdw: Disable batch insert when BEFORE ROW INSERT triggers exist. ... Previously, we allowed this, but such triggers might query the table to insert into and act differently if the tuples that have already been processed and prepared for insertion are not there, so disable it in such cases. Back-patch to v14 where batch insert was added. Discussion: https://postgr.es/m/CAPmGK16_uPqsmgK0-LpLSUk54_BoK13bPrhxhfjSoSTVz414hA%40mail.gmail.com	3 years ago
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
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
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
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
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
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
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
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
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
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
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
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
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
Tomas Vondra	99cea49d65	Fix copying data into slots with FDW batching ... Commit b676ac443b optimized handling of tuple slots with bulk inserts into foreign tables, so that the slots are initialized only once and reused for all batches. The data was however copied into the slots only after the initialization, inserting duplicate values when the slot gets reused. Fixed by moving the ExecCopySlot outside the init branch. The existing postgres_fdw tests failed to catch this due to inserting data into foreign tables without unique indexes, and then checking only the number of inserted rows. This adds a new test with both a unique index and a check of inserted values. Reported-by: Alexander Pyhalov Discussion: https://postgr.es/m/7a8cf8d56b3d18e5c0bccd6cd42d04ac%40postgrespro.ru	4 years ago
Tomas Vondra	cb92703384	Adjust batch size in postgres_fdw to not use too many parameters ... The FE/BE protocol identifies parameters with an Int16 index, which limits the maximum number of parameters per query to 65535. With batching added to postges_fdw this limit is much easier to hit, as the whole batch is essentially a single query, making this error much easier to hit. The failures are a bit unpredictable, because it also depends on the number of columns in the query. So instead of just failing, this patch tweaks the batch_size to not exceed the maximum number of parameters. Reported-by: Hou Zhijie <houzj.fnst@cn.fujitsu.com> Reviewed-by: Bharath Rupireddy <bharath.rupireddyforpostgres@gmail.com> Discussion: https://postgr.es/m/OS0PR01MB571603973C0AC2874AD6BF2594299%40OS0PR01MB5716.jpnprd01.prod.outlook.com	4 years ago
Etsuro Fujita	f3baaf28a6	Fix rescanning of async-aware Append nodes. ... In cases where run-time pruning isn't required, the synchronous and asynchronous subplans for an async-aware Append node determined using classify_matching_subplans() should be re-used when rescanning the node, but the previous code re-determined them using that function repeatedly each time when rescanning the node, leading to incorrect results in a normal build and an Assert failure in an Assert-enabled build as that function doesn't assume that it's called repeatedly in such cases. Fix the code as mentioned above. My oversight in commit 27e1f1456. While at it, initialize async-related pointers/variables to NULL/zero explicitly in ExecInitAppend() and ExecReScanAppend(), just to be sure. (The variables would have been set to zero before we get to the latter function, but let's do so.) Reviewed-by: Kyotaro Horiguchi Discussion: https://postgr.es/m/CAPmGK16Q4B2_KY%2BJH7rb7wQbw54AUprp7TMekGTd2T1B62yysQ%40mail.gmail.com	4 years ago
Tom Lane	f61db909df	Fix postgres_fdw failure with whole-row Vars of type RECORD. ... Commit 86dc90056 expects that FDWs can cope with whole-row Vars for their tables, even if the Vars are marked with vartype RECORDOID. Previously, whole-row Vars generated by the planner had vartype equal to the relevant table's rowtype OID. (The point behind this change is to enable sharing of resjunk columns across inheritance child tables.) It turns out that postgres_fdw fails to cope with this, though through bad fortune none of its test cases exposed that. Things mostly work, but when we try to read back a value of such a Var, the expected rowtype is not available to record_in(). Fortunately, it's not difficult to hack up the tupdesc that controls this process to substitute the foreign table's rowtype for RECORDOID. Thus we can solve the runtime problem while still sharing the resjunk column with other tables. Per report from Alexander Pyhalov. Discussion: https://postgr.es/m/7817fb9ebd6661cdf9b67dec6e129a78@postgrespro.ru	4 years ago
Tom Lane	889592344c	Fix planner's row-mark code for inheritance from a foreign table. ... Commit 428b260f8 broke planning of cases where row marks are needed (SELECT FOR UPDATE, etc) and one of the query's tables is a foreign table that has regular table(s) as inheritance children. We got the reverse case right, but apparently were thinking that foreign tables couldn't be inheritance parents. Not so; so we need to be able to add a CTID junk column while adding a new child, not only a wholerow junk column. Back-patch to v12 where the faulty code came in. Amit Langote Discussion: https://postgr.es/m/CA+HiwqEmo3FV1LAQ4TVyS2h1WM=kMkZUmbNuZSCnfHvMcUcPeA@mail.gmail.com	4 years ago