Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/04/08 7:44, Tom Lane wrote: > Etsuro Fujita writes: >> To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like >> to propose the following FDW APIs: > >> RowMarkType >> GetForeignRowMarkType(Oid relid, >> LockClauseStrength strength); > >> Decide which rowmark type to use for a foreign table (that has strength >> = LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY. (For now, the >> second argument takes LCS_NONE only, but is intended to be used for the >> possible extension to the other cases.) This is called during >> select_rowmark_type() in the planner. > > Why would you restrict that to LCS_NONE? Seems like the point is to give > the FDW control of the rowmark behavior, not only partial control. The reason is because I think it's comparatively more promissing to customize the ROW_MARK type for LCS_NONE than other cases since in many workloads no re-fetch is needed, and because I think other cases would need more discussions. So, as a first step, I restricted that to LCS_NONE. But I've got the point, and agree that we should give the full control to the FDW. > (For the same reason I disagree with the error check in the patch that > restricts which ROW_MARK options this function can return. If the FDW has > TIDs, seems like it could reasonably use whatever options tables can use.) Will fix. >> void >> BeginForeignFetch(EState *estate, >> ExecRowMark *erm, >> List *fdw_private, >> int eflags); > >> Begin a remote fetch. This is called during InitPlan() in the executor. > > The begin/end functions seem like useless extra mechanism. Why wouldn't > the FDW just handle this during its regular scan setup? It could look to > see whether the foreign table is referenced by any ExecRowMarks (possibly > there's room to add some convenience functions to help with that). What's > more, that design would make it simpler if the basic row fetch needs to be > modified. The reason is just because it's easy to understand the structure at least to me since the begin/exec/end are all done in a higher level of the executor. What's more, the begin/end can be done once per foreign scan node for the multi-table update case. But I feel inclined to agree with you on this point also. >> And I'd also like to propose to add a table/server option, >> row_mark_reference, to postgres_fdw. When a user sets the option to >> true for eg a foreign table, ROW_MARK_REFERENCE will be used for the >> table, not ROW_MARK_COPY. > > Why would we leave that in the hands of the user? Hardly anybody is going > to know what to do with the option, or even that they should do something > with it. It's basically only of value for debugging AFAICS, Agreed. (When begining to update postgres_fdw docs, I also started to think so.) I'll update the patch, which will contain only an infrastructure for this in the PG core, and will not contain any postgres_fdw change. Thank you for taking the time to review the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/13 0:50, Tom Lane wrote:
I think the real fix as far as postgres_fdw is concerned is in fact
to let it adopt a different ROW_MARK strategy, since it has meaningful
ctid values. However, that is not a one-size-fits-all answer.
The tableoid problem can be fixed much less invasively as per the attached
patch. I think that we should continue to assume that ctid is not
meaningful (and hence should read as (4294967295,0)) in FDWs that use
ROW_MARK_COPY, and press forward on fixing the locking issues for
postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to
that. That would also cause ctid to read properly for rows from
postgres_fdw.
To support ROW_MARK_REFERENCE on (postgres_fdw) foreign tables, I'd like
to propose the following FDW APIs:
RowMarkType
GetForeignRowMarkType(Oid relid,
LockClauseStrength strength);
Decide which rowmark type to use for a foreign table (that has strength
= LCS_NONE), ie, ROW_MARK_REFERENCE or ROW_MARK_COPY. (For now, the
second argument takes LCS_NONE only, but is intended to be used for the
possible extension to the other cases.) This is called during
select_rowmark_type() in the planner.
void
BeginForeignFetch(EState *estate,
ExecRowMark *erm,
List *fdw_private,
int eflags);
Begin a remote fetch. This is called during InitPlan() in the executor.
HeapTuple
ExecForeignFetch(EState *estate,
ExecRowMark *erm,
ItemPointer tupleid);
Re-fetch the specified tuple. This is called during
EvalPlanQualFetchRowMarks() in the executor.
void
EndForeignFetch(EState *estate,
ExecRowMark *erm);
End a remote fetch. This is called during ExecEndPlan() in the executor.
And I'd also like to propose to add a table/server option,
row_mark_reference, to postgres_fdw. When a user sets the option to
true for eg a foreign table, ROW_MARK_REFERENCE will be used for the
table, not ROW_MARK_COPY.
Attached is a WIP patch, which contains no docs/regression tests.
It'd be appreciated if anyone could send back any comments earlier.
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/option.c
--- b/contrib/postgres_fdw/option.c
***
*** 105,111 postgres_fdw_validator(PG_FUNCTION_ARGS)
* Validate option value, when we can do so without any context.
*/
if (strcmp(def->defname, "use_remote_estimate") == 0 ||
! strcmp(def->defname, "updatable") == 0)
{
/* these accept only boolean values */
(void) defGetBoolean(def);
--- 105,112
* Validate option value, when we can do so without any context.
*/
if (strcmp(def->defname, "use_remote_estimate") == 0 ||
! strcmp(def->defname, "updatable") == 0 ||
! strcmp(def->defname, "row_mark_reference") == 0)
{
/* these accept only boolean values */
(void) defGetBoolean(def);
***
*** 153,158 InitPgFdwOptions(void)
--- 154,162
/* updatable is available on both server and table */
{"updatable", ForeignServerRelationId, false},
{"updatable", ForeignTableRelationId, false},
+ /* row_mark_reference is available on both server and table */
+ {"row_mark_reference", ForeignServerRelationId, false},
+ {"row_mark_reference", ForeignTableRelationId, false},
{NULL, InvalidOid, false}
};
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 124,129 enum FdwModifyPrivateIndex
--- 124,144
};
/*
+ * Similarly, this enum describes what's kept in the fdw_private list for
+ * a PlanRowMark node referencing a postgres_fdw foreign table. We store:
+ *
+ * 1) SELECT statement text to be sent to the remote server
+ * 2) Integer list of attribute numbers retrieved by SELECT
+ */
+ enum FdwFetchPrivateIndex
+ {
+ /* SQL statement to execute remotely (as a String node) */
+ FdwFetchPrivateSelectSql,
+ /* Integer list of attribute numbers retrieved by SELECT */
+ FdwFetchPrivateRetrievedAttrs
+ };
+
+ /*
* Execution state of a foreign scan using postgres_fdw.
*/
typedef struct PgFdwScanState
***
*** 186,191 typedef struct PgFdwModifyState
--- 201,230
} PgFdwModifyState;
/*
+ * Execution state of a foreign fetch operation.
+ */
+ typedef struct PgFdwFetchState
+ {
+ Relation rel; /* relcache entry for the foreign table */
+ AttInMetadata *attinmeta; /* attribute datatype conversion metadata */
+
+ /* for remote query execution */
+ PGconn *conn; /* connection for the fetch */
+ char *p_name; /* name of prepared statement, if created */
+
+ /* extracted fdw_private data */
+ char *query; /* text of SELECT command */
+ List *retrieved_attrs; /* attr numbers retrieved by SELECT */
+
+ /* info about parameters for p
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/25 4:56, Tom Lane wrote: > Etsuro Fujita writes: >> Let me explain further. Here is the comment in ExecOpenScanRelation: > >>* Determine the lock type we need. First, scan to see if target >> relation >>* is a result relation. If not, check if it's a FOR UPDATE/FOR SHARE >>* relation. In either of those cases, we got the lock already. > >> I think this is not true for foreign tables selected FOR UPDATE/SHARE, >> which have markType = ROW_MARK_COPY, because such foreign tables don't >> get opened/locked by InitPlan. Then such foreign tables don't get >> locked by neither of InitPlan nor ExecOpenScanRelation. I think this is >> a bug. > > You are right. I think it may not matter in practice, but if the executor > is taking its own locks here then it should not overlook ROW_MARK_COPY > cases. > >> To fix it, I think we should open/lock such foreign tables at >> InitPlan as the original patch does. > > I still don't like that; InitPlan is not doing something that would > require physical table access. The right thing is to fix > ExecOpenScanRelation's idea of whether InitPlan took a lock or not, > which I've now done. OK, thanks. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2015/03/23 2:57, Tom Lane wrote: > I've committed this with some substantial rearrangements, notably: Thanks for taking the time to committing the patch! Thanks for the work, Hanada-san! And thank you everyone for the reviews and comments, especially Ashutosh, Horiguchi-san and Noah! > * I fooled around with the PlanRowMark changes some more, mainly with > the idea that we might soon allow FDWs to use rowmark methods other than > ROW_MARK_COPY. The planner now has just one place where a rel's rowmark > method is chosen, so as to centralize anything we need to do there. Will work on this issue. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typos in CREATE TABLE doc
On 2015/03/21 5:58, Bruce Momjian wrote: On Thu, Nov 13, 2014 at 08:30:49PM +0900, Etsuro Fujita wrote: (2014/11/13 20:07), Heikki Linnakangas wrote: On 11/13/2014 12:45 PM, Etsuro Fujita wrote: It seems to me there are typos in the reference page for CREATE TABLE. The structure of the sentence is a bit funky, but it seems correct to me. If you google for "should any", you'll get a bunch of pages discussing similar sentences. I would add a comma there, though: "Should any row of an insert or update operation produce a FALSE result, an exception is raised and ..." I understand. So, Here is the comma patch. Patch applied. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Incorrect comment in tablecmds.c
On 2015/03/20 21:31, Bruce Momjian wrote: On Thu, Oct 23, 2014 at 06:29:07PM +0900, Etsuro Fujita wrote: I don't think that the lock level mentioned in the following comment in MergeAttributes() in tablecmds.c is right, since that that function has opened the relation with ShareUpdateExclusiveLock, not with AccessShareLock. Patch attached. 1749 /* 1750 * Close the parent rel, but keep our AccessShareLock on it until xact 1751 * commit. That will prevent someone else from deleting or ALTERing 1752 * the parent before the child is committed. 1753 */ 1754 heap_close(relation, NoLock); Agreed, patch applied. Thanks. Thanks for picking this up! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Future directions for inheritance-hierarchy statistics
On 2015/03/17 5:18, Tom Lane wrote: > A few days ago I posted a very-much-WIP patch for making the planner > dynamically combine statistics for each member of an appendrel: > http://www.postgresql.org/message-id/22598.1425686...@sss.pgh.pa.us > > That patch was only intended to handle the case of an appendrel generated > by a UNION ALL construct. But it occurs to me that we could easily > change it to also apply to appendrels generated from inheritance trees. > Then we'd no longer need the whole-inheritance-tree statistics that > ANALYZE currently produces, because we'd only ever look at per-table > statistics in pg_statistic. > > This would have one significant drawback, which is that planning for > large inheritance trees (many children) would probably get noticeably > slower. (But in the common case that constraint exclusion limits a > query to scanning just one or a few children, the hit would be small.) > > On the other hand, there would be two very significant benefits. > First, that we would automatically get statistics that account for > partitions being eliminated by constraint exclusion, because only the > non-eliminated partitions are present in the appendrel. And second, > that we'd be able to forget the whole problem of getting autovacuum > to create whole-inheritance-tree stats. Right now I'm doubtful that > typical users are getting good up-to-date stats at all for queries of > this sort, because autovacuum will only update those stats if it decides > it needs to analyze the parent table. Which is commonly empty, so that > there's never a reason to fire an analyze on it. (We'd left this as > a problem to be solved later when we put in the whole-tree stats > feature in 9.0, but no progress has been made on solving it.) > > So I think that going in this direction is clearly a win and we ought > to pursue it. It's not happening for 9.5 of course, because there's > still a great deal of work to do before anything like this would be > committable. But I would like to establish a consensus that this > would be a sensible thing to do in 9.6. > > The reason I bring it up now is that the inheritance-for-foreign-tables > patch has some code that I don't much like for controlling what happens > with those whole-tree stats when some of the children are foreign tables > that lack ANALYZE support. If the long-term plan is that whole-tree > stats are going away altogether, then it won't be terribly important > exactly what happens in that case, so we can just do some simple/easy > kluge in the short term and not have to have an argument about what's > the best thing to do. That seems like a good idea. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: Custom/Foreign-Join-APIs (Re: [HACKERS] [v9.5] Custom Plan API)
On 2015/03/14 7:18, Robert Haas wrote: I think the foreign data wrapper join pushdown case, which also aims to substitute a scan for a join, is interesting to think about, even though it's likely to be handled by a new FDW method instead of via the hook. Where should the FDW method get called from? I haven't had enough time to review the patch in details yet, so I don't know where we should call the method, but I'd vote for the idea of substituting a scan for a join, because I think that idea would probably allow update pushdown, which I'm proposing in the current CF, to scale up to handling a pushed-down update on a join. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/13 11:46, Etsuro Fujita wrote: BTW, what do you think about opening/locking foreign tables selected for update at InitPlan, which the original patch does? As I mentioned in [1], ISTM that ExecOpenScanRelation called from ExecInitForeignScan is assuming that. [1] http://www.postgresql.org/message-id/54bcbbf8.3020...@lab.ntt.co.jp Let me explain further. Here is the comment in ExecOpenScanRelation: * Determine the lock type we need. First, scan to see if target relation * is a result relation. If not, check if it's a FOR UPDATE/FOR SHARE * relation. In either of those cases, we got the lock already. I think this is not true for foreign tables selected FOR UPDATE/SHARE, which have markType = ROW_MARK_COPY, because such foreign tables don't get opened/locked by InitPlan. Then such foreign tables don't get locked by neither of InitPlan nor ExecOpenScanRelation. I think this is a bug. To fix it, I think we should open/lock such foreign tables at InitPlan as the original patch does. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/13 0:50, Tom Lane wrote: The tableoid problem can be fixed much less invasively as per the attached patch. I think that we should continue to assume that ctid is not meaningful (and hence should read as (4294967295,0)) in FDWs that use ROW_MARK_COPY, and press forward on fixing the locking issues for postgres_fdw by letting it use ROW_MARK_REFERENCE or something close to that. That would also cause ctid to read properly for rows from postgres_fdw. OK, thanks! BTW, what do you think about opening/locking foreign tables selected for update at InitPlan, which the original patch does? As I mentioned in [1], ISTM that ExecOpenScanRelation called from ExecInitForeignScan is assuming that. Best regards, Etsuro Fujita [1] http://www.postgresql.org/message-id/54bcbbf8.3020...@lab.ntt.co.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/12 13:35, Tom Lane wrote:
> I don't like the execMain.c changes much at all. They look somewhat
> like they're intended to allow foreign tables to adopt a different
> locking strategy,
I didn't intend such a thing. My intention is, foreign tables have
markType = ROW_MARK_COPY as ever, but I might not have correctly
understood what you pointed out. Could you elaborate on that?
> The changes are not all consistent
> either, eg this:
>
> ! if (erm->relation &&
> ! erm->relation->rd_rel->relkind != RELKIND_FOREIGN_TABLE)
>
> is not necessary if this Assert is accurate:
>
> ! if (erm->relation)
> ! {
> ! Assert(erm->relation->rd_rel->relkind ==
> RELKIND_FOREIGN_TABLE);
I modified InitPlan so that relations get opened for foreign tables
as well as local tables. So, I think the change would be necessary.
> I don't see the need for the change in nodeForeignscan.c. If the FDW has
> returned a physical tuple, it can fix that for itself, while if it has
> returned a virtual tuple, the ctid will be garbage in any case.
If you leave nodeForeignscan.c unchanged, file_fdw would cause the
problem that I pointed out upthread. Here is an example.
[Create a test environment]
postgres=# create foreign table foo (a int) server file_server options
(filename '/home/pgsql/foo.csv', format 'csv');
CREATE FOREIGN TABLE
postgres=# select tableoid, ctid, * from foo;
tableoid | ctid | a
--++---
16459 | (4294967295,0) | 1
(1 row)
postgres=# create table tab (a int, b int);
CREATE TABLE
postgres=# insert into tab values (1, 1);
INSERT 0 1
[Run concurrent transactions]
In terminal1:
postgres=# begin;
BEGIN
postgres=# update tab set b = b * 2;
UPDATE 1
In terminal2:
postgres=# select foo.tableoid, foo.ctid, foo.* from foo, tab where
foo.a = tab.a for update;
In terminal1:
postgres=# commit;
COMMIT
In terminal2:(When committing the UPDATE query in terminal1, psql in
terminal2 will display the result for the SELECT-FOR-UPDATE query as
shown below.)
tableoid | ctid | a
--+---+---
16459 | (0,0) | 1
(1 row)
Note the value of the ctid has changed!
Rather than changing nodeForeignscan.c, it might be better to change
heap_form_tuple to set the t_ctid of a formed tuple to be invalid.
Thanks for the review!
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/11 17:37, Ashutosh Bapat wrote: Now I can reproduce the problem. Sanity Patch compiles cleanly and make check passes. The tests in file_fdw and postgres_fdw contrib modules pass. The patch works as expected in the test case reported. Thanks for the testing! I have only one doubt. In EvalPlanQualFetchRowMarks(). tuple->t_self is assigned from td->t_ctid. CTID or even t_self may be valid for foreign tables based on postgres_fdw but may not be valid for other FDWs. So, this assignment might put some garbage in t_self, rather we should set it to invalid as done prior to the patch. I might have missed some previous thread, we decided to go this route, so ignore the comment, in that case. Good point. As the following code and comment I added to ForeignNext, I think that FDW authors should initialize the tup->t_data->t_ctid of each scan tuple with its own TID. If the authors do that, the t_self is guaranteed to be assigned the right TID from the whole-row Var (ie, td->t_ctid) in EvalPlanQualFetchRowMarks. /* We assume that t_ctid is initialized with its own TID */ tup->t_self = tup->t_data->t_ctid; IMHO, I'm not sure it's worth complicating the code as you mentioned. (I don't know whether there are any discussions about this before.) Note that file_fdw needs no treatment. In that case, in ForeignNext, the tup->t_data->t_ctid of each scan tuple is initialized with (0,0) (if necessary), and then the t_self will be correctly assigned (0,0) throguh the whole-row Var in EvalPlanQualFetchRowMarks. So, no inconsistency! Apart from this, I do not have any comments here. Thanks again. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/03/09 16:02, Ashutosh Bapat wrote: I tried reproducing the issue with the steps summarised. Thanks for the review! Here's my setup Sorry, my explanation was not enough, but such was not my intention. I'll re-summarize the steps below: [Create a test environment] $ createdb mydatabase $ psql mydatabase mydatabase=# create table mytable (a int); $ psql postgres postgres=# create extension postgres_fdw; postgres=# create server loopback foreign data wrapper postgres_fdw options (dbname 'mydatabase'); postgres=# create user mapping for current_user server loopback; postgres=# create foreign table ftable (a int) server loopback options (table_name 'mytable'); postgres=# insert into ftable values (1); postgres=# create table ltable (a int, b int); postgres=# insert into ltable values (1, 1); [Run concurrent transactions] In terminal1: $ psql postgres postgres=# begin; BEGIN postgres=# update ltable set b = b * 2; UPDATE 1 In terminal2: $ psql postgres postgres=# select tableoid, ctid, * from ftable; tableoid | ctid | a --+---+--- 16394 | (0,1) | 1 (1 row) postgres=# select f.tableoid, f.ctid, f.* from ftable f, ltable l where f.a = l.a for update; In terminal1: postgres=# commit; COMMIT In terminal2:(When committing the UPDATE query in terminal1, psql in terminal2 will display the result for the SELECT-FOR-UPDATE query as shown below.) tableoid | ctid | a --++--- 0 | (4294967295,0) | 1 (1 row) Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
On 2015/03/04 17:07, Etsuro Fujita wrote:
On 2015/03/04 16:58, Albe Laurenz wrote:
Etsuro Fujita wrote:
While updating the patch, I noticed that in the previous patch, there is
a bug in pushing down parameterized UPDATE/DELETE queries; generic plans
for such queries fail with a can't-happen error. I fixed the bug and
tried to add the regression tests that execute the generic plans, but I
couldn't because I can't figure out how to force generic plans. Is
there any way to do that?
I don't know about a way to force it, but if you run the statement six
times, it will probably switch to a generic plan.
Yeah, I was just thinking running the statement six times in the
regression tests ...
Here is an updated version. In this version, the bug has been fixed,
but any regression tests for that hasn't been added, because I'm not
sure that the above way is a good idea and don't have any other ideas.
The EXPLAIN output has also been improved as discussed in [1].
On top of this, I'll try to extend the join push-down patch to support a
pushed-down update on a join, though I'm still digesting the series of
patches.
Comments welcome.
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/31942.1410534...@sss.pgh.pa.us
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 189,198 is_foreign_expr(PlannerInfo *root,
if (!foreign_expr_walker((Node *) expr, &glob_cxt, &loc_cxt))
return false;
- /* Expressions examined here should be boolean, ie noncollatable */
- Assert(loc_cxt.collation == InvalidOid);
- Assert(loc_cxt.state == FDW_COLLATE_NONE);
-
/*
* An expression which includes any mutable functions can't be sent over
* because its result is not stable. For example, sending now() remote
--- 189,194
***
*** 788,794 deparseTargetList(StringInfo buf,
*
* If params is not NULL, it receives a list of Params and other-relation Vars
* used in the clauses; these values must be transmitted to the remote server
! * as parameter values.
*
* If params is NULL, we're generating the query for EXPLAIN purposes,
* so Params and other-relation Vars should be replaced by dummy values.
--- 784,791
*
* If params is not NULL, it receives a list of Params and other-relation Vars
* used in the clauses; these values must be transmitted to the remote server
! * as parameter values. Caller is responsible for initializing the result list
! * to empty if necessary.
*
* If params is NULL, we're generating the query for EXPLAIN purposes,
* so Params and other-relation Vars should be replaced by dummy values.
***
*** 805,813 appendWhereClause(StringInfo buf,
int nestlevel;
ListCell *lc;
- if (params)
- *params = NIL; /* initialize result list to empty */
-
/* Set up context struct for recursion */
context.root = root;
context.foreignrel = baserel;
--- 802,807
***
*** 940,945 deparseUpdateSql(StringInfo buf, PlannerInfo *root,
--- 934,996
}
/*
+ * deparse remote UPDATE statement
+ *
+ * The statement text is appended to buf, and we also create an integer List
+ * of the columns being retrieved by RETURNING (if any), which is returned
+ * to *retrieved_attrs.
+ */
+ void
+ deparsePushedDownUpdateSql(StringInfo buf, PlannerInfo *root,
+ Index rtindex, Relation rel,
+ List *targetlist,
+ List *targetAttrs,
+ List *remote_conds,
+ List **params_list,
+ List *returningList,
+ List **retrieved_attrs)
+ {
+ RelOptInfo *baserel = root->simple_rel_array[rtindex];
+ deparse_expr_cxt context;
+ bool first;
+ ListCell *lc;
+
+ if (params_list)
+ *params_list = NIL; /* initialize result list to empty */
+
+ /* Set up context struct for recursion */
+ context.root = root;
+ context.foreignrel = baserel;
+ context.buf = buf;
+ context.params_list = params_list;
+
+ appendStringInfoString(buf, "UPDATE ");
+ deparseRelation(buf, rel);
+ appendStringInfoString(buf, " SET ");
+
+ first = true;
+ foreach(lc, targetAttrs)
+ {
+ int attnum = lfirst_int(lc);
+ TargetEntry *tle = get_tle_by_resno(targetlist, attnum);
+
+ if (!first)
+ appendStringInfoString(buf, ", ");
+ first = false;
+
+ deparseColumnRef(buf, rtindex, attnum, root);
+ appendStringInfo(buf, " = ");
+ deparseExpr((Expr *) tle->expr, &context);
+ }
+ if (remote_conds)
+ appendWhereClause(buf, root, baserel, remote_conds,
+ true, params_list);
+
+ deparseReturningList(buf, root, rtindex, rel, false,
+ returningList, retrieved_attrs);
+ }
+
+ /*
* deparse remote DELETE statement
*
* The statement text is appended to buf, and we also create an integer List
***
*** 962,967 deparseDeleteSq
Re: [HACKERS] Join push-down support for foreign tables
On 2015/03/04 17:57, Shigeru Hanada wrote: 2015-03-04 17:00 GMT+09:00 Etsuro Fujita : On 2015/03/03 21:34, Shigeru Hanada wrote: I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join v6 patch. but still the patch has an issue about joins underlying UPDATE or DELETE. Now I'm working on fixing this issue. Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar are remote? If so, I think it'd be better to push such an update down to the remote, as discussed in [2], and I'd like to work on that together! A part of it, perhaps. But at the moment I see many issues to solve around pushing down complex UPDATE/DELETE. So I once tightened the restriction, that joins between foreign tables are pushed down only if they are part of SELECT statement. Please see next v4 patch I'll post soon. OK, thanks for the reply! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Join push-down support for foreign tables
On 2015/03/04 17:31, Kouhei Kaigai wrote: On 2015/03/03 21:34, Shigeru Hanada wrote: I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join v6 patch. Maybe I'm missing something, but did we agree to take this approach, ie, "join push-down" on top of custom join? There is a comment ahout that [1]. I just thought it'd be better to achieve a consensus before implementing the feature further. It is not correct. The join push-down feature is not implemented on top of the custom-join feature, however, both of them are 99% similar on both of the concept and implementation. So, we're working to enhance foreign/custom-join interface together, according to Robert's suggestion [3], using postgres_fdw extension as a minimum worthwhile example for both of foreign/custom-scan. OK, thanks for the explanation! but still the patch has an issue about joins underlying UPDATE or DELETE. Now I'm working on fixing this issue. Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar are remote? If so, I think it'd be better to push such an update down to the remote, as discussed in [2], and I'd like to work on that together! Hanada-san, could you give us test query to reproduce the problem above? I and Fujita-san can help to investigate the problem from different standpoints for each. Yeah, will do. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
On 2015/03/04 16:58, Albe Laurenz wrote: Etsuro Fujita wrote: While updating the patch, I noticed that in the previous patch, there is a bug in pushing down parameterized UPDATE/DELETE queries; generic plans for such queries fail with a can't-happen error. I fixed the bug and tried to add the regression tests that execute the generic plans, but I couldn't because I can't figure out how to force generic plans. Is there any way to do that? I don't know about a way to force it, but if you run the statement six times, it will probably switch to a generic plan. Yeah, I was just thinking running the statement six times in the regression tests ... Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Join push-down support for foreign tables
On 2015/03/03 21:34, Shigeru Hanada wrote: I rebased "join push-down" patch onto Kaigai-san's Custom/Foreign Join v6 patch. Thanks for the work, Hanada-san and KaiGai-san! Maybe I'm missing something, but did we agree to take this approach, ie, "join push-down" on top of custom join? There is a comment ahout that [1]. I just thought it'd be better to achieve a consensus before implementing the feature further. but still the patch has an issue about joins underlying UPDATE or DELETE. Now I'm working on fixing this issue. Is that something like "UPDATE foo ... FROM bar ..." where both foo and bar are remote? If so, I think it'd be better to push such an update down to the remote, as discussed in [2], and I'd like to work on that together! Sorry for having been late for the party. Best regards, Etsuro Fujita [1] http://www.postgresql.org/message-id/23343.1418658...@sss.pgh.pa.us [2] http://www.postgresql.org/message-id/31942.1410534...@sss.pgh.pa.us -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
On 2015/02/16 12:03, Etsuro Fujita wrote: > I'll update the patch. While updating the patch, I noticed that in the previous patch, there is a bug in pushing down parameterized UPDATE/DELETE queries; generic plans for such queries fail with a can't-happen error. I fixed the bug and tried to add the regression tests that execute the generic plans, but I couldn't because I can't figure out how to force generic plans. Is there any way to do that? Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/03/02 5:28, Stephen Frost wrote: * Dean Rasheed (dean.a.rash...@gmail.com) wrote: I just spotted a trivial bug in this patch -- in expand_security_quals() you need to set targetRelation = false inside the loop, otherwise it will be true for the target relation and all that follow it. I've pushed a fix for this. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/26 11:38, Stephen Frost wrote: I've pushed an update for this to master and 9.4 and improved the comments and the commit message as discussed. Would be great if you could test and let me know if you run into any issues! OK, thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2015/01/15 16:35, Etsuro Fujita wrote:
> On 2014/12/23 0:36, Tom Lane wrote:
>> Yeah, we need to do something about the PlanRowMark data structure.
>> Aside from the pre-existing issue in postgres_fdw, we need to fix it
>> to support inheritance trees in which more than one rowmark method
>> is being used. That rte.hasForeignChildren thing is a crock,
>
>> The idea I'd had about that was to convert the markType field into a
>> bitmask, so that a parent node's markType could represent the logical
>> OR of the rowmark methods being used by all its children.
> As I said before, that seems to me like a good idea. So I'll update the
> patch based on that if you're okey with it.
Done based on your ideas: (a) add a field to PlanRowMark to record the
original lock strength to fix the postgres_fdw issue and (b) convert its
markType field into a bitmask to support the inheritance trees. I think
that both work well and that (a) is useful for the other places.
Patch attached, which has been created on top of [1].
Best regards,
Etsuro Fujita
[1] http://www.postgresql.org/message-id/54bcbbf8.3020...@lab.ntt.co.jp
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
--- 148,167
SELECT * FROM agg_csv WHERE a < 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a < 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table "agg_csv"
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table "agg_csv"
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,"test triggered !")
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ b | bbb
+ b |
+ b | b
+ b | bb
+ b | bbb
+ b | bbb
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/18 21:44, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: On 2015/02/18 7:44, Stephen Frost wrote: Attached is a patch which should address this. Would love your (or anyone else's) feedback on it. It appears to address the issue which you raised and the regression test changes are all in-line with inserting a LockRows into the subquery, as anticipated. I've looked into the patch. * The patch applies to the latest head, 'make' passes successfully, but 'make check' fails in the rowsecurity test. Apologies for not being clear- the patch was against 9.4, where it passes all the regression tests (at least for me- if you see differently, please let me know!). Sorry, I assumed that the patch was against HEAD. I comfermed that the back-patched 9.4 passes all the regression tests! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
On 2015/02/18 8:13, Tom Lane wrote: > So I went back to your v1 patch and have now committed that with some > cosmetic modifications. Sorry for making you put time into a dead end. I don't mind it. Thanks for committing the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/18 7:44, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: On 2015/02/11 4:06, Stephen Frost wrote: I had been trying to work out an FDW-specific way to address this, but I think Dean's right that this should be addressed in expand_security_qual(), which means it'll apply to all cases and not just these FDW calls. I don't think that's actually an issue though and it'll match up to how SELECT FOR UPDATE is handled today. Sorry, my explanation was not accurate, but I also agree with Dean's idea. In the above, I just wanted to make it clear that such a lock request done by expand_security_qual() should be limited to the case where the relation that is a former result relation is a foreign table. Attached is a patch which should address this. Would love your (or anyone else's) feedback on it. It appears to address the issue which you raised and the regression test changes are all in-line with inserting a LockRows into the subquery, as anticipated. I've looked into the patch. * The patch applies to the latest head, 'make' passes successfully, but 'make check' fails in the rowsecurity test. * I found one place in expand_security_qual that I'm concerned about: + if (targetRelation) + applyLockingClause(subquery, 1, LCS_FORUPDATE, + false, false); ISTM that it'd be better to use LockWaitBlock as the fourth argument of applyLockingClause. Other than that, the patch looks good to me. Thanks for the work! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Optimization for updating foreign tables in Postgres FDW
On 2014/09/13 0:13, Tom Lane wrote: > Albe Laurenz writes: >> Tom Lane wrote: >>> I'm not sure offhand what the new plan tree ought to look like. We could >>> just generate a ForeignScan node, but that seems like rather a misnomer. >>> Is it worth inventing a new ForeignUpdate node type? Or maybe it'd still >>> be ForeignScan but with a flag field saying "hey this is really an update >>> (or a delete)". The main benefit I can think of right now is that the >>> EXPLAIN output would be less strange-looking --- but EXPLAIN is hardly >>> the only thing that ever looks at plan trees, so having an outright >>> misleading plan structure is likely to burn us down the line. > >> I can understand these qualms. >> I wonder if "ForeignUpdate" is such a good name though, since it would >> surprise the uninitiate that in the regular (no push-down) case the >> actual modification is *not* performed by ForeignUpdate. >> So it should rather be a "ForeignModifyingScan", but I personally would >> prefer a "has_side_effects" flag on ForeignScan. > > I was envisioning that the EXPLAIN output would look like > > Foreign Scan on tab1 >Remote SQL: SELECT ... > > for the normal case, versus > > Foreign Update on tab1 >Remote SQL: UPDATE ... > > for the pushed-down-update case (and similarly for DELETE). For a > non-optimized update it'd still be a ForeignScan underneath a ModifyTable. > > As for the internal representation, I was thinking of adding a CmdType > field to struct ForeignScan, with currently only CMD_SELECT, CMD_UPDATE, > CMD_DELETE as allowed values, though possibly in future we'd think of a > reason to allow CMD_INSERT there. This is more or less isomorphic to your > "has_side_effects" flag, but it allows distinguishing UPDATE from DELETE > which might be useful. > > The only thing that's bothering me about this concept is that I'm not > seeing how to scale it up to handling a pushed-down update on a join, > ie, "UPDATE foo ... FROM bar ..." where both foo and bar are remote. > Maybe it's silly to worry about that until join push-down is done; > but in that case I'd vote for postponing this whole patch until we > have join push-down. I'll re-add this to the final CF. And I'll update the patch. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/11 4:06, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: On 2015/02/10 7:23, Dean Rasheed wrote: Sorry, I didn't have time to look at this properly. My initial thought is that expand_security_qual() needs to request a lock on rows coming >from the relation it pushes down into a subquery if that relation was the result relation, because otherwise it won't have any locks, since preprocess_rowmarks() only adds PlanRowMarks to non-target relations. That seems close to what I had in mind; expand_security_qual() needs to request a FOR UPDATE lock on rows coming from the relation it pushes down into a subquery only when that relation is the result relation and *foreign table*. I had been trying to work out an FDW-specific way to address this, but I think Dean's right that this should be addressed in expand_security_qual(), which means it'll apply to all cases and not just these FDW calls. I don't think that's actually an issue though and it'll match up to how SELECT FOR UPDATE is handled today. Sorry, my explanation was not accurate, but I also agree with Dean's idea. In the above, I just wanted to make it clear that such a lock request done by expand_security_qual() should be limited to the case where the relation that is a former result relation is a foreign table. If it's OK, I'll submit a patch for that, maybe early next week. Thank you for working on this issue! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
On 2015/02/10 14:49, Etsuro Fujita wrote:
> On 2015/02/07 1:09, Tom Lane wrote:
>> IIRC, this code was written at a time when we didn't have NO INHERIT check
>> constraints and so it was impossible for the parent table to get optimized
>> away while leaving children. So the comment in ExplainModifyTarget was
>> good at the time. But it no longer is.
>>
>> I think your basic idea of preserving the original parent table's relid
>> is correct; but instead of doing it like this patch does, I'd be inclined
>> to make ModifyTable inherit from Scan not Plan, and use the scan.scanrelid
>> field to carry the parent RTI. Then you would probably end up with a net
>> savings of code rather than net addition; certainly ExplainModifyTarget
>> would go away entirely since you'd just treat ModifyTable like any other
>> Scan in this part of EXPLAIN.
>
> Will follow your revision.
Done. Attached is an updated version of the patch.
Best regards,
Etsuro Fujita
*** a/src/backend/commands/explain.c
--- b/src/backend/commands/explain.c
***
*** 95,101 static const char *explain_get_index_name(Oid indexId);
static void ExplainIndexScanDetails(Oid indexid, ScanDirection indexorderdir,
ExplainState *es);
static void ExplainScanTarget(Scan *plan, ExplainState *es);
- static void ExplainModifyTarget(ModifyTable *plan, ExplainState *es);
static void ExplainTargetRel(Plan *plan, Index rti, ExplainState *es);
static void show_modifytable_info(ModifyTableState *mtstate, ExplainState *es);
static void ExplainMemberNodes(List *plans, PlanState **planstates,
--- 95,100
***
*** 724,736 ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
- *rels_used = bms_add_member(*rels_used,
- ((Scan *) plan)->scanrelid);
- break;
case T_ModifyTable:
- /* cf ExplainModifyTarget */
*rels_used = bms_add_member(*rels_used,
! linitial_int(((ModifyTable *) plan)->resultRelations));
break;
default:
break;
--- 723,731
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
case T_ModifyTable:
*rels_used = bms_add_member(*rels_used,
! ((Scan *) plan)->scanrelid);
break;
default:
break;
***
*** 1067,1072 ExplainNode(PlanState *planstate, List *ancestors,
--- 1062,1068
case T_WorkTableScan:
case T_ForeignScan:
case T_CustomScan:
+ case T_ModifyTable:
ExplainScanTarget((Scan *) plan, es);
break;
case T_IndexScan:
***
*** 1101,1109 ExplainNode(PlanState *planstate, List *ancestors,
ExplainPropertyText("Index Name", indexname, es);
}
break;
- case T_ModifyTable:
- ExplainModifyTarget((ModifyTable *) plan, es);
- break;
case T_NestLoop:
case T_MergeJoin:
case T_HashJoin:
--- 1097,1102
***
*** 2104,2127 ExplainScanTarget(Scan *plan, ExplainState *es)
}
/*
- * Show the target of a ModifyTable node
- */
- static void
- ExplainModifyTarget(ModifyTable *plan, ExplainState *es)
- {
- Index rti;
-
- /*
- * We show the name of the first target relation. In multi-target-table
- * cases this should always be the parent of the inheritance tree.
- */
- Assert(plan->resultRelations != NIL);
- rti = linitial_int(plan->resultRelations);
-
- ExplainTargetRel((Plan *) plan, rti, es);
- }
-
- /*
* Show the target relation of a scan or modify node
*/
static void
--- 2097,2102
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***
*** 120,125 CopyPlanFields(const Plan *from, Plan *newnode)
--- 120,140
}
/*
+ * CopyScanFields
+ *
+ * This function copies the fields of the Scan node. It is used by
+ * all the copy functions for classes which inherit from Scan.
+ */
+ static void
+ CopyScanFields(const Scan *from, Scan *newnode)
+ {
+ CopyPlanFields((const Plan *) from, (Plan *) newnode);
+
+ COPY_SCALAR_FIELD(scanrelid);
+ }
+
+
+ /*
* _copyPlan
*/
static Plan *
***
*** 168,174 _copyModifyTable(const ModifyTable *from)
/*
* copy node superclass fields
*/
! CopyPlanFields((const Plan *) from, (Plan *) newnode);
/*
* copy remainder of node
--- 183,189
/*
* copy node superclass fields
*/
! CopyScanFields((const Scan *) from, (Scan *) newnode);
/*
* copy remainder of node
***
*** 306,325 _copyBitmapOr(const BitmapOr *from)
/*
- * CopyScanFields
- *
- * This function copies the fields of the Scan node. It is used by
- * all the copy functions for classes which inherit from Scan.
- */
- static void
- CopyScanFields(const Scan *from, Scan *newnode)
- {
- CopyPlanFields((const Plan *) from, (Pla
Re: [HACKERS] Odd behavior of updatable security barrier views on foreign tables
On 2015/02/10 7:23, Dean Rasheed wrote: On 9 February 2015 at 21:17, Stephen Frost wrote: On Fri, Jan 30, 2015 at 5:20 AM, Etsuro Fujita I noticed that when updating security barrier views on foreign tables, we fail to give FOR UPDATE to selection queries issued at ForeignScan. I've looked into this a fair bit more over the weekend and the issue appears to be that the FDW isn't expecting a do-instead sub-query. I've been considering how we might be able to address that but havn't come up with any particularly great ideas and would welcome any suggestions. Simply having the FDW try to go up through the query would likely end up with too many queries showing up with 'for update'. We add the 'for update' to the sub-query before we even get called from the 'Modify' path too, which means we can't use that to realize when we're getting ready to modify rows and therefore need to lock them. In any case, I'll continue to look but would welcome any other thoughts. Sorry, I didn't have time to look at this properly. My initial thought is that expand_security_qual() needs to request a lock on rows coming from the relation it pushes down into a subquery if that relation was the result relation, because otherwise it won't have any locks, since preprocess_rowmarks() only adds PlanRowMarks to non-target relations. That seems close to what I had in mind; expand_security_qual() needs to request a FOR UPDATE lock on rows coming from the relation it pushes down into a subquery only when that relation is the result relation and *foreign table*. Thanks for dicussing this issue! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
On 2015/02/07 1:09, Tom Lane wrote: > IIRC, this code was written at a time when we didn't have NO INHERIT check > constraints and so it was impossible for the parent table to get optimized > away while leaving children. So the comment in ExplainModifyTarget was > good at the time. But it no longer is. > > I think your basic idea of preserving the original parent table's relid > is correct; but instead of doing it like this patch does, I'd be inclined > to make ModifyTable inherit from Scan not Plan, and use the scan.scanrelid > field to carry the parent RTI. Then you would probably end up with a net > savings of code rather than net addition; certainly ExplainModifyTarget > would go away entirely since you'd just treat ModifyTable like any other > Scan in this part of EXPLAIN. Will follow your revision. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
Hi Ashutosh, On 2015/02/03 16:44, Ashutosh Bapat wrote: I am having some minor problems running this repro [Terminal 2] postgres=# create foreign table ft (a int) server loopback options (table_name 'lbt'); There isn't any table "lbt" mentioned here. Do you mean "t" here? Sorry, my explanation was not enough. "lbt" means a foreign table named "lbt" defined on a foreign server named "loopback". It'd be defined eg, in the following manner: $ createdb efujita efujita=# create table lbt (a int); CREATE TABLE postgres=# create server loopback foreign data wrapper postgres_fdw options (dbname 'efujita'); CREATE SERVER postgres=# create user mapping for current_user server loopback; CREATE USER MAPPING postgres=# create foreign table ft (a int) server loopback options (table_name 'lbt'); CREATE FOREIGN TABLE Thanks for the review! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
Hi Ashutosh, Thank you for the review! On 2015/02/03 15:32, Ashutosh Bapat wrote: I agree that it's a problem, and it looks more severe when there are multiple children postgres=# create table parent (a int check (a < 0) no inherit); CREATE TABLE postgres=# create table child1 (a int check (a >= 0)); CREATE TABLE postgres=# create table child2 (a int check (a >= 0)); CREATE TABLE postgres=# create table child3 (a int check (a >= 0)); CREATE TABLE postgres=# alter table child1 inherit parent; ALTER TABLE postgres=# alter table child2 inherit parent; ALTER TABLE postgres=# alter table child3 inherit parent; ALTER TABLE postgres=# explain update parent set a = a * 2 where a >= 0; QUERY PLAN Update on child1 (cost=0.00..126.00 rows=2400 width=10) -> Seq Scan on child1 (cost=0.00..42.00 rows=800 width=10) Filter: (a >= 0) -> Seq Scan on child2 (cost=0.00..42.00 rows=800 width=10) Filter: (a >= 0) -> Seq Scan on child3 (cost=0.00..42.00 rows=800 width=10) Filter: (a >= 0) (7 rows) It's certainly confusing why would an update on child1 cause scan on child*. Yeah, I think so too. But I also think that showing parent's name with Upate would be misleading esp. when user expects it to get filtered because of constraint exclusion. Instead, can we show all the relations that are being modified e.g Update on child1, child2, child3. That will disambiguate everything. That's an idea, but my concern about that is the cases where there are a large number of child tables as the EXPLAIN would be difficult to read in such cases. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Odd behavior of updatable security barrier views on foreign tables
Hi, I noticed that when updating security barrier views on foreign tables, we fail to give FOR UPDATE to selection queries issued at ForeignScan. Here is an example. postgres=# create foreign table base_ftbl (person text, visibility text) server loopback options (table_name 'base_tbl'); CREATE FOREIGN TABLE postgres=# create view rw_view as select person from base_ftbl where visibility = 'public'; CREATE VIEW postgres=# explain verbose delete from rw_view; QUERY PLAN --- Delete on public.base_ftbl (cost=100.00..144.40 rows=14 width=6) Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1 -> Foreign Scan on public.base_ftbl (cost=100.00..144.40 rows=14 width=6) Output: base_ftbl.ctid Remote SQL: SELECT ctid FROM public.base_tbl WHERE ((visibility = 'public'::text)) FOR UPDATE (5 rows) postgres=# alter view rw_view set (security_barrier = true); ALTER VIEW postgres=# explain verbose delete from rw_view; QUERY PLAN -- Delete on public.base_ftbl base_ftbl_1 (cost=100.00..144.54 rows=14 width=6) Remote SQL: DELETE FROM public.base_tbl WHERE ctid = $1 -> Subquery Scan on base_ftbl (cost=100.00..144.54 rows=14 width=6) Output: base_ftbl.ctid -> Foreign Scan on public.base_ftbl base_ftbl_2 (cost=100.00..144.40 rows=14 width=6) Output: base_ftbl_2.ctid Remote SQL: SELECT ctid FROM public.base_tbl WHERE ((visibility = 'public'::text)) (7 rows) Correct me if I am wrong. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/01/19 17:10, Etsuro Fujita wrote: Attached is an updated version of the patch. I'll add this to the next CF. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] ExplainModifyTarget doesn't work as expected
On 2015/01/27 9:15, Jim Nasby wrote: On 12/22/14 12:50 AM, Etsuro Fujita wrote: I think ExplainModifyTarget should show the parent of the inheritance tree in multi-target-table cases, as described there, but noticed that it doesn't always work like that. Here is an example. Anything ever happen with this? Nothing. I'll add this to the next CF. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Another comment typo in src/backend/executor/execMain.c
On 2015/01/20 1:44, Robert Haas wrote: On Mon, Jan 19, 2015 at 4:00 AM, Etsuro Fujita wrote: I ran into another typo in execMain.c. Patch attached. Thanks, committed! Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Another comment typo in src/backend/executor/execMain.c
Hi, I ran into another typo in execMain.c. Patch attached. Best regards, Etsuro Fujita diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index fcc42fa..bc910f0 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2182,7 +2182,7 @@ EvalPlanQualInit(EPQState *epqstate, EState *estate, /* * EvalPlanQualSetPlan -- set or change subplan of an EPQState. * - * We need this so that ModifyTuple can deal with multiple subplans. + * We need this so that ModifyTable can deal with multiple subplans. */ void EvalPlanQualSetPlan(EPQState *epqstate, Plan *subplan, List *auxrowmarks) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
On 2015/01/16 1:24, Alvaro Herrera wrote:
Etsuro Fujita wrote:
*** 817,826 InitPlan(QueryDesc *queryDesc, int eflags)
--- 818,833
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
+ relkind = rt_fetch(rc->rti,
rangeTable)->relkind;
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ relid = getrelid(rc->rti, rangeTable);
+ else
+ relid = InvalidOid;
relation = NULL;
break;
--- 2326,2342
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! /* if relid is valid, rel is a foreign table; set
system columns */
! if (OidIsValid(erm->relid))
! {
! tuple.t_self = td->t_ctid;
! tuple.t_tableOid = erm->relid;
! }
! else
! {
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
! }
tuple.t_data = td;
I find this arrangement confusing and unnecessary -- surely if you have
access to the ExecRowMark here, you could just obtain the relid with
RelationGetRelid instead of saving the OID beforehand?
I don't think so because we don't have the Relation (ie, erm->relation =
NULL) here since InitPlan don't open/lock relations having markType =
ROW_MARK_COPY as shown above, which include foreign tables selected for
update/share. But I noticed we should open/lock such foreign tables as
well in InitPlan because I think ExecOpenScanRelation is assuming that,
IIUC.
> And if you have
the Relation, you could just consult the relkind at that point instead
of relying on the relid being set or not as a flag to indicate whether
the rel is foreign.
Followed your revision. Attached is an updated version of the patch.
Thanks for the comment!
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 2947,2953 make_tuple_from_result_row(PGresult *res,
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
--- 2947,2953
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = tuple->t_data->t_ctid = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 795,800 InitPlan(QueryDesc *queryDesc, int eflags)
--- 795,801
{
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
Oid relid;
+ char relkind;
Relation relation;
ExecRowMark *erm;
***
*** 817,823 InitPlan(QueryDesc *queryDesc, int eflags)
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relation = NULL;
break;
default:
elog(ERROR, "unrecognized markType: %d", rc->markType);
--- 818,831
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
! relkind = rt_fetch(rc->rti, rangeTable)->relkind;
! if (relkind == RELKIND_FOREIGN_TABLE)
! {
! relid = getrelid(rc->rti, rangeTable);
! relation = heap_open(relid, AccessShareLock);
! }
! else
! relation = NULL;
break;
default:
elog(ERROR, "unrecognized markType: %d", rc->markType);
***
*** 1115,1125 CheckValidRowMarkRel(Relation rel, RowMarkType markType)
RelationGetRelationName(rel;
break;
case RELKIND_FOREIGN_TABLE:
! /* Should not get here; planner should have used ROW_MARK_COPY */
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("cannot lock rows in foreign table \"%s\"",
! RelationGetRelationName(rel;
break;
default:
ereport(ERROR,
--- 1123,1134
RelationGetRelationName(rel;
break;
case RELKIND_FOREIGN_TABLE:
! /* Allow opening/locking a foreign table only if ROW_MARK_COPY */
! if (markType != ROW_MARK_COPY)
! ereport(ERROR,
! (errcode(ERRCODE_WRONG_OBJECT_TYPE),
! errmsg("cannot lock rows in foreign table \"%s\"",
! RelationGetRelationName(rel;
break;
default:
ereport(ERROR,
***
*** 1792,1798 ExecBuildAuxRowMark(ExecRowMark *erm, List *targetlist)
Re: [HACKERS] inherit support for foreign tables
On 2014/12/23 0:36, Tom Lane wrote: > Yeah, we need to do something about the PlanRowMark data structure. > Aside from the pre-existing issue in postgres_fdw, we need to fix it > to support inheritance trees in which more than one rowmark method > is being used. That rte.hasForeignChildren thing is a crock, > The idea I'd had about that was to convert the markType field into a > bitmask, so that a parent node's markType could represent the logical > OR of the rowmark methods being used by all its children. I've not > attempted to code this up though, and probably won't get to it until > after Christmas. One thing that's not clear is what should happen > with ExecRowMark. As I said before, that seems to me like a good idea. So I'll update the patch based on that if you're okey with it. Or you've found any problem concerning the above idea? Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] EvalPlanQual behaves oddly for FDW queries involving system columns
Here is an example using postgres_fdw.
[Terminal 1]
postgres=# create table t (a int, b int);
CREATE TABLE
postgres=# insert into t values (1, 1);
INSERT 0 1
postgres=# begin;
BEGIN
postgres=# update t set b = b * 2;
UPDATE 1
[Terminal 2]
postgres=# create foreign table ft (a int) server loopback options
(table_name 'lbt');
CREATE FOREIGN TABLE
postgres=# insert into ft values (1);
INSERT 0 1
postgres=# select tableoid, ctid, * from ft;
tableoid | ctid | a
--+---+---
25092 | (0,1) | 1
(1 row)
postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;
[Terminal 1]
postgres=# commit;
COMMIT
[Terminal 2]
postgres=# select ft.tableoid, ft.ctid, ft.* from t, ft where t.a = ft.a
for update;
tableoid | ctid | a
--++---
0 | (4294967295,0) | 1
(1 row)
Note that tableoid and ctid have been changed!
I think the reason for that is because EvalPlanQualFetchRowMarks doesn't
properly set tableoid and ctid for foreign tables, IIUC. I think this
should be fixed. Please find attached a patch. The patch slightly
relates to [1], so if it is reasonable, I'll update [1] on top of this.
[1] https://commitfest.postgresql.org/action/patch_view?id=1386
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 2947,2953 make_tuple_from_result_row(PGresult *res,
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
--- 2947,2953
tuple = heap_form_tuple(tupdesc, values, nulls);
if (ctid)
! tuple->t_self = tuple->t_data->t_ctid = *ctid;
/* Clean up */
MemoryContextReset(temp_context);
*** a/src/backend/executor/execMain.c
--- b/src/backend/executor/execMain.c
***
*** 795,800 InitPlan(QueryDesc *queryDesc, int eflags)
--- 795,801
{
PlanRowMark *rc = (PlanRowMark *) lfirst(l);
Oid relid;
+ char relkind;
Relation relation;
ExecRowMark *erm;
***
*** 817,826 InitPlan(QueryDesc *queryDesc, int eflags)
--- 818,833
break;
case ROW_MARK_COPY:
/* there's no real table here ... */
+ relkind = rt_fetch(rc->rti, rangeTable)->relkind;
+ if (relkind == RELKIND_FOREIGN_TABLE)
+ relid = getrelid(rc->rti, rangeTable);
+ else
+ relid = InvalidOid;
relation = NULL;
break;
default:
elog(ERROR, "unrecognized markType: %d", rc->markType);
+ relid = InvalidOid;
relation = NULL; /* keep compiler quiet */
break;
}
***
*** 831,836 InitPlan(QueryDesc *queryDesc, int eflags)
--- 838,844
erm = (ExecRowMark *) palloc(sizeof(ExecRowMark));
erm->relation = relation;
+ erm->relid = relid;
erm->rti = rc->rti;
erm->prti = rc->prti;
erm->rowmarkId = rc->rowmarkId;
***
*** 2318,2325 EvalPlanQualFetchRowMarks(EPQState *epqstate)
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
tuple.t_data = td;
/* copy and store tuple */
--- 2326,2342
/* build a temporary HeapTuple control structure */
tuple.t_len = HeapTupleHeaderGetDatumLength(td);
! /* if relid is valid, rel is a foreign table; set system columns */
! if (OidIsValid(erm->relid))
! {
! tuple.t_self = td->t_ctid;
! tuple.t_tableOid = erm->relid;
! }
! else
! {
! ItemPointerSetInvalid(&(tuple.t_self));
! tuple.t_tableOid = InvalidOid;
! }
tuple.t_data = td;
/* copy and store tuple */
*** a/src/backend/executor/nodeForeignscan.c
--- b/src/backend/executor/nodeForeignscan.c
***
*** 22,27
--- 22,28
*/
#include "postgres.h"
+ #include "access/htup_details.h"
#include "executor/executor.h"
#include "executor/nodeForeignscan.h"
#include "foreign/fdwapi.h"
***
*** 53,65 ForeignNext(ForeignScanState *node)
/*
* If any system columns are requested, we have to force the tuple into
* physical-tuple form to avoid "cannot extract system attribute from
! * virtual tuple" errors later. We also insert a valid value for
! * tableoid, which is the only actually-useful system column.
*/
if (plan->fsSystemCol && !TupIsNull(slot))
{
HeapTuple tup = ExecMaterializeSlot(slot);
tup->t_tableOid = RelationGetRelid(node->ss.ss_currentRelation);
}
--- 54,69
/*
* If any system columns are requested, we have to force the tuple into
* physical-tuple form to avoid "cannot extract system attribute from
! * virtua
Re: [HACKERS] Comment typo in src/backend/executor/execMain.c
On 2015/01/10 1:08, Stephen Frost wrote: * Etsuro Fujita (fujita.ets...@lab.ntt.co.jp) wrote: I ran into a comment type. Please find attached a patch. Fix pushed. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Comment typo in src/backend/executor/execMain.c
Hi, I ran into a comment type. Please find attached a patch. Best regards, Etsuro Fujita diff --git a/src/backend/executor/execMain.c b/src/backend/executor/execMain.c index 8c799d3..28abfa4 100644 --- a/src/backend/executor/execMain.c +++ b/src/backend/executor/execMain.c @@ -2024,7 +2024,7 @@ EvalPlanQualFetch(EState *estate, Relation relation, int lockmode, * heap_lock_tuple() will throw an error, and so would any later * attempt to update or delete the tuple. (We need not check cmax * because HeapTupleSatisfiesDirty will consider a tuple deleted - * by our transaction dead, regardless of cmax.) Wee just checked + * by our transaction dead, regardless of cmax.) We just checked * that priorXmax == xmin, so we can test that variable instead of * doing HeapTupleHeaderGetXmin again. */ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
On 2014/12/23 0:36, Tom Lane wrote:
> Yeah, we need to do something about the PlanRowMark data structure.
> Aside from the pre-existing issue in postgres_fdw, we need to fix it
> to support inheritance trees in which more than one rowmark method
> is being used. That rte.hasForeignChildren thing is a crock, and
> would still be a crock even if it were correctly documented as being
> a planner temporary variable (rather than the current implication that
> it's always valid). RangeTblEntry is no place for planner temporaries.
Agreed.
> The idea I'd had about that was to convert the markType field into a
> bitmask, so that a parent node's markType could represent the logical
> OR of the rowmark methods being used by all its children. I've not
> attempted to code this up though, and probably won't get to it until
> after Christmas. One thing that's not clear is what should happen
> with ExecRowMark.
That seems like a good idea, as parent PlanRowMarks are ignored at runtime.
Aside from the above, I noticed that the patch has a bug in handling
ExecRowMarks/ExecAuxRowMarks for foreign tables in inheritance trees
during the EPQ processing.:-( Attached is an updated version of the
patch to fix that, which has been created on top of [1], as said before.
Thanks,
[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
--- 148,167
SELECT * FROM agg_csv WHERE a < 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a < 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table "agg_csv"
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table "agg_csv"
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,"test triggered !")
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ --
Re: [HACKERS] inherit support for foreign tables
On 2014/12/18 7:04, Tom Lane wrote:
> Etsuro Fujita writes:
>> Attached are updated patches. Patch fdw-inh-5.patch has been created on
>> top of patch fdw-chk-5.patch.
> I've committed fdw-chk-5.patch with some minor further adjustments;
> Have not looked at the other patch yet.
I updated the remaining patch correspondingly to the fix [1]. Please
find attached a patch (the patch has been created on top of the patch in
[1]).
I haven't done anything about the issue that postgresGetForeignPlan() is
using get_parse_rowmark(), discussed in eg, [2]. Tom, will you work on
that?
Thanks,
[1] http://www.postgresql.org/message-id/5497bf4c.6080...@lab.ntt.co.jp
[2] http://www.postgresql.org/message-id/18256.1418401...@sss.pgh.pa.us
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 148,153 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
--- 148,167
SELECT * FROM agg_csv WHERE a < 0;
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ SELECT tableoid::regclass, * FROM agg_csv;
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ DELETE FROM agg WHERE a = 100;
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 249,254 SELECT * FROM agg_csv WHERE a < 0;
--- 249,294
(0 rows)
RESET constraint_exclusion;
+ -- table inheritance tests
+ CREATE TABLE agg (a int2, b float4);
+ ALTER FOREIGN TABLE agg_csv INHERIT agg;
+ SELECT tableoid::regclass, * FROM agg;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM agg_csv;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ SELECT tableoid::regclass, * FROM ONLY agg;
+ tableoid | a | b
+ --+---+---
+ (0 rows)
+
+ -- updates aren't supported
+ UPDATE agg SET a = 1;
+ ERROR: cannot update foreign table "agg_csv"
+ DELETE FROM agg WHERE a = 100;
+ ERROR: cannot delete from foreign table "agg_csv"
+ -- but this should be ignored
+ SELECT tableoid::regclass, * FROM agg FOR UPDATE;
+ tableoid | a |b
+ --+-+-
+ agg_csv | 100 | 99.097
+ agg_csv | 0 | 0.09561
+ agg_csv | 42 | 324.78
+ (3 rows)
+
+ ALTER FOREIGN TABLE agg_csv NO INHERIT agg;
+ DROP TABLE agg;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 3027,3032 NOTICE: NEW: (13,"test triggered !")
--- 3027,3544
(1 row)
-- ===
+ -- test inheritance features
+ -- ===
+ CREATE TABLE a (aa TEXT);
+ CREATE TABLE loct (aa TEXT, bb TEXT);
+ CREATE FOREIGN TABLE b (bb TEXT) INHERITS (a)
+ SERVER loopback OPTIONS (table_name 'loct');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO a(aa) VALUES('a');
+ INSERT INTO a(aa) VALUES('aa');
+ INSERT INTO a(aa) VALUES('aaa');
+ INSERT INTO a(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ INSERT INTO b(aa) VALUES('b');
+ INSERT INTO b(aa) VALUES('bb');
+ INSERT INTO b(aa) VALUES('bbb');
+ INSERT INTO b(aa) VALUES('');
+ SELECT relname, a.* FROM a, pg_class where a.tableoid = pg_class.oid;
+ relname |aa
+ -+--
+ a | aaa
+ a |
+ a | a
+ a | aa
+ a | aaa
+ a |
+ b | bbb
+ b |
+ b | b
+ b | bb
+ b | bbb
+ b |
+ (12 rows)
+
+ SELECT relname, b.* FROM b, pg_class where b.tableoid = pg_class.oid;
+ relname |aa| bb
+ -+--+
+ b | bbb |
+ b | |
+ b | b|
+ b | bb |
+ b | bbb |
+ b | |
+ (6 rows)
+
+ SELECT relname, a.* FROM ONLY a, pg_class where a.tableoid = pg_class.oid;
+ relname |
[HACKERS] ExplainModifyTarget doesn't work as expected
Hi,
I think ExplainModifyTarget should show the parent of the inheritance
tree in multi-target-table cases, as described there, but noticed that
it doesn't always work like that. Here is an example.
postgres=# create table parent (a int check (a < 0) no inherit);
CREATE TABLE
postgres=# create table child (a int check (a >= 0));
CREATE TABLE
postgres=# alter table child inherit parent;
ALTER TABLE
postgres=# explain update parent set a = a * 2 where a >= 0;
QUERY PLAN
---
Update on child (cost=0.00..42.00 rows=800 width=10)
-> Seq Scan on child (cost=0.00..42.00 rows=800 width=10)
Filter: (a >= 0)
(3 rows)
IIUC, I think this is because ExplainModifyTarget doesn't take into
account that the parent *can* be excluded by constraint exclusion. So,
I added a field to ModifyTable to record the parent, apart from
resultRelations. (More precisely, the parent in its role as a simple
member of the inheritance tree is recorded so that appending digits to
refname in select_rtable_names_for_explain works as before.) Attached
is a proposed patch for that.
Thanks,
Best regards,
Etsuro Fujita
*** a/src/backend/commands/explain.c
--- b/src/backend/commands/explain.c
***
*** 728,736 ExplainPreScanNode(PlanState *planstate, Bitmapset **rels_used)
((Scan *) plan)->scanrelid);
break;
case T_ModifyTable:
- /* cf ExplainModifyTarget */
*rels_used = bms_add_member(*rels_used,
! linitial_int(((ModifyTable *) plan)->resultRelations));
break;
default:
break;
--- 728,735
((Scan *) plan)->scanrelid);
break;
case T_ModifyTable:
*rels_used = bms_add_member(*rels_used,
! ((ModifyTable *) plan)->resultRelation);
break;
default:
break;
***
*** 2109,2122 ExplainScanTarget(Scan *plan, ExplainState *es)
static void
ExplainModifyTarget(ModifyTable *plan, ExplainState *es)
{
! Index rti;
!
! /*
! * We show the name of the first target relation. In multi-target-table
! * cases this should always be the parent of the inheritance tree.
! */
! Assert(plan->resultRelations != NIL);
! rti = linitial_int(plan->resultRelations);
ExplainTargetRel((Plan *) plan, rti, es);
}
--- 2108,2114
static void
ExplainModifyTarget(ModifyTable *plan, ExplainState *es)
{
! Index rti = plan->resultRelation;
ExplainTargetRel((Plan *) plan, rti, es);
}
*** a/src/backend/nodes/copyfuncs.c
--- b/src/backend/nodes/copyfuncs.c
***
*** 175,180 _copyModifyTable(const ModifyTable *from)
--- 175,181
*/
COPY_SCALAR_FIELD(operation);
COPY_SCALAR_FIELD(canSetTag);
+ COPY_SCALAR_FIELD(resultRelation);
COPY_NODE_FIELD(resultRelations);
COPY_SCALAR_FIELD(resultRelIndex);
COPY_NODE_FIELD(plans);
*** a/src/backend/nodes/outfuncs.c
--- b/src/backend/nodes/outfuncs.c
***
*** 327,332 _outModifyTable(StringInfo str, const ModifyTable *node)
--- 327,333
WRITE_ENUM_FIELD(operation, CmdType);
WRITE_BOOL_FIELD(canSetTag);
+ WRITE_UINT_FIELD(resultRelation);
WRITE_NODE_FIELD(resultRelations);
WRITE_INT_FIELD(resultRelIndex);
WRITE_NODE_FIELD(plans);
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 4809,4814 make_result(PlannerInfo *root,
--- 4809,4815
ModifyTable *
make_modifytable(PlannerInfo *root,
CmdType operation, bool canSetTag,
+ Index resultRelation,
List *resultRelations, List *subplans,
List *withCheckOptionLists, List *returningLists,
List *rowMarks, int epqParam)
***
*** 4857,4862 make_modifytable(PlannerInfo *root,
--- 4858,4864
node->operation = operation;
node->canSetTag = canSetTag;
+ node->resultRelation = resultRelation;
node->resultRelations = resultRelations;
node->resultRelIndex = -1; /* will be set correctly in setrefs.c */
node->plans = subplans;
*** a/src/backend/optimizer/plan/planner.c
--- b/src/backend/optimizer/plan/planner.c
***
*** 607,612 subquery_planner(PlannerGlobal *glob, Query *parse,
--- 607,613
plan = (Plan *) make_modifytable(root,
parse->commandType,
parse->canSetTag,
+ parse->resultRelation,
list_make1_int(parse->resultRelation),
list_make1(plan),
withCheckOptionLists,
***
*** 790,795 inheritance_planner(PlannerInfo *root)
--- 791,797
{
Query *parse = root->parse;
int parentRTindex = parse->resultRelation;
+ int pseudoParentRTindex = -1;
List *final_rtable = NIL;
int save_rel_array_size = 0;
RelOptInfo **save_rel_array = NULL;
***
*** 925,930 inheritance_planner(PlannerInf
Re: [HACKERS] Minor improvement to explain.c
(2014/12/18 17:34), Fujii Masao wrote: On Thu, Dec 18, 2014 at 12:52 PM, Etsuro Fujita wrote: The attached patch just removes one bad-looking blank line in the comments at the top of a function in explain.c. Applied. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Minor improvement to explain.c
Hi, The attached patch just removes one bad-looking blank line in the comments at the top of a function in explain.c. Thanks, Best regards, Etsuro Fujita diff --git a/src/backend/commands/explain.c b/src/backend/commands/explain.c index 332f04a..064f880 100644 --- a/src/backend/commands/explain.c +++ b/src/backend/commands/explain.c @@ -568,7 +568,6 @@ ExplainPrintPlan(ExplainState *es, QueryDesc *queryDesc) /* * ExplainPrintTriggers - - * convert a QueryDesc's trigger statistics to text and append it to * es->str * -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/18 7:04), Tom Lane wrote: > Etsuro Fujita writes: >> Attached are updated patches. Patch fdw-inh-5.patch has been created on >> top of patch fdw-chk-5.patch. Patch fdw-chk-5.patch is basically the >> same as the previous one fdw-chk-4.patch, but I slightly modified that >> one. The changes are the following. >> * added to foreign_data.sql more tests for your comments. >> * revised docs on ALTER FOREIGN TABLE a bit further. > > I've committed fdw-chk-5.patch with some minor further adjustments; > the most notable one was that I got rid of the error check prohibiting > NO INHERIT, which did not seem to me to have any value. Attaching such > a clause won't have any effect, but so what? > > Have not looked at the other patch yet. Thanks! I added the error check because the other patch, fdw-inh-5.patch, doesn't allow foreign tables to be inherited and so it seems more consistent at least to me to do so. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/16 2:59), Tom Lane wrote: > Etsuro Fujita writes: >> (2014/12/13 1:17), Tom Lane wrote: >>> We should >>> probably also think about allowing FDWs to change these settings if >>> they want to. > >> This is not clear to me. Maybe I'm missing something, but I think that >> the FDW only needs to look at the original locking strength in >> GetForeignPlan(). Please explain that in a little more detail. > > Well, the point is that for postgres_fdw, we could consider using the > same locked-row-identification methods as for local tables, ie CTID. > Now admittedly this might be the only such case, but I'm not entirely > convinced of that --- you could imagine using FDWs for many of the same > use-cases that KaiGai-san has been proposing custom scans for, and > in some of those cases CTIDs would be useful for row IDs. > > We'd originally dismissed this on the argument that ROWMARK_REFERENCE > is a cheaper implementation than CTID-based implementations for any > FDW (since it avoids the possible need to fetch a remote row twice). > I'm not sure I still believe that though. Fetching all columns of all > retrieved rows in order to avoid what might be zero or a small number of > re-fetches is not obviously a win, especially not for FDWs that > represent not-actually-remote resources. > > So as long as we're revisiting this area, it might be worth thinking > about letting an FDW have some say in which ROWMARK method is selected > for its tables. Understood. So, to what extext should we consider such things in the FDW improvement? We've already started an independent infrastructure for such things, ie, custom scans, IIUC. Thank you for the explanation! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/13 1:17), Tom Lane wrote: > Etsuro Fujita writes: >>> (2014/12/12 10:37), Tom Lane wrote: >>>> Yeah, this is clearly a thinko: really, nothing in the planner should >>>> be using get_parse_rowmark(). I looked around for other errors of the >>>> same type and found that postgresGetForeignPlan() is also using >>>> get_parse_rowmark(). While that's harmless at the moment because we >>>> don't support foreign tables as children, it's still wrong. >> In order >> to get the locking strength, I think we need to see the RowMarkClauses >> and thus still need to use get_parse_rowmark() in >> postgresGetForeignPlan(), though I agree with you that that is ugly. > I think this needs more thought; I'm still convinced that having the FDW > look at the parse rowmarks is the Wrong Thing. However, we don't need > to solve it in existing branches. With 9.4 release so close, the right > thing is to revert that change for now and consider a HEAD-only patch > later. OK > (One idea is to go ahead and make a ROW_MARK_COPY item, but > add a field to PlanRowMark to record the original value. +1 > We should > probably also think about allowing FDWs to change these settings if > they want to. This is not clear to me. Maybe I'm missing something, but I think that the FDW only needs to look at the original locking strength in GetForeignPlan(). Please explain that in a little more detail. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/12 11:33), Etsuro Fujita wrote: > (2014/12/12 11:19), Tom Lane wrote: >> Etsuro Fujita writes: >>> (2014/12/12 10:37), Tom Lane wrote: >>>> Yeah, this is clearly a thinko: really, nothing in the planner should >>>> be using get_parse_rowmark(). I looked around for other errors of the >>>> same type and found that postgresGetForeignPlan() is also using >>>> get_parse_rowmark(). While that's harmless at the moment because we >>>> don't support foreign tables as children, it's still wrong. Will >>>> fix that too. >> >>> I don't think we need to fix that too. In order to support that, I'm >>> proposing to modify postgresGetForeignPlan() in the following way [1] >>> (see fdw-inh-5.patch). >> >> My goodness, that's ugly. And it's still wrong, because this is planner >> code so it shouldn't be using get_parse_rowmark at all. The whole point >> here is that the rowmark info has been transformed into something >> appropriate for the planner to use. While that transformation is >> relatively trivial today, it might not always be so. > > OK, I'll update the inheritance patch on top of the revison you'll make. Thanks for your speedy work. While updating the inheritance patch, I noticed that the fix for postgresGetForeignPlan() is not right. Since PlanRowMarks for foreign tables get the ROW_MARK_COPY markType during preprocess_rowmarks(), so we can't get the locking strength from the PlanRowMarks, IIUC. In order to get the locking strength, I think we need to see the RowMarkClauses and thus still need to use get_parse_rowmark() in postgresGetForeignPlan(), though I agree with you that that is ugly. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/12 11:19), Tom Lane wrote: > Etsuro Fujita writes: >> (2014/12/12 10:37), Tom Lane wrote: >>> Yeah, this is clearly a thinko: really, nothing in the planner should >>> be using get_parse_rowmark(). I looked around for other errors of the >>> same type and found that postgresGetForeignPlan() is also using >>> get_parse_rowmark(). While that's harmless at the moment because we >>> don't support foreign tables as children, it's still wrong. Will >>> fix that too. > >> I don't think we need to fix that too. In order to support that, I'm >> proposing to modify postgresGetForeignPlan() in the following way [1] >> (see fdw-inh-5.patch). > > My goodness, that's ugly. And it's still wrong, because this is planner > code so it shouldn't be using get_parse_rowmark at all. The whole point > here is that the rowmark info has been transformed into something > appropriate for the planner to use. While that transformation is > relatively trivial today, it might not always be so. OK, I'll update the inheritance patch on top of the revison you'll make. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [Bug] Inconsistent result for inheritance and FOR UPDATE.
(2014/12/12 10:37), Tom Lane wrote:
> Yeah, this is clearly a thinko: really, nothing in the planner should
> be using get_parse_rowmark(). I looked around for other errors of the
> same type and found that postgresGetForeignPlan() is also using
> get_parse_rowmark(). While that's harmless at the moment because we
> don't support foreign tables as children, it's still wrong. Will
> fix that too.
I don't think we need to fix that too. In order to support that, I'm
proposing to modify postgresGetForeignPlan() in the following way [1]
(see fdw-inh-5.patch).
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 824,834 postgresGetForeignPlan(PlannerInfo *root,
{
RowMarkClause *rc = get_parse_rowmark(root->parse,
baserel->relid);
if (rc)
{
/*
!* Relation is specified as a FOR UPDATE/SHARE target,
so handle
!* that.
*
* For now, just ignore any [NO] KEY specification,
since (a) it's
* not clear what that means for a remote table that we
don't have
--- 824,847
{
RowMarkClause *rc = get_parse_rowmark(root->parse,
baserel->relid);
+ /*
+* It's possible that relation is contained in an inheritance
set and
+* that parent relation is selected FOR UPDATE/SHARE. If so,
get the
+* RowMarkClause for parent relation.
+*/
+ if (rc == NULL)
+ {
+ PlanRowMark *prm = get_plan_rowmark(root->rowMarks,
baserel->relid);
+
+ if (prm && prm->rti != prm->prti)
+ rc = get_parse_rowmark(root->parse, prm->prti);
+ }
+
if (rc)
{
/*
!* Relation or parent relation is specified as a FOR
UPDATE/SHARE
!* target, so handle that.
*
* For now, just ignore any [NO] KEY specification,
since (a) it's
* not clear what that means for a remote table that we
don't have
[1] http://www.postgresql.org/message-id/5487bb9d.7070...@lab.ntt.co.jp
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/11 14:54), Ashutosh Bapat wrote: I marked this as ready for committer. Many thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hi Ashutosh, Thanks for the review! (2014/12/10 14:47), Ashutosh Bapat wrote: We haven't heard anything from Horiguchi-san and Hanada-san for almost a week. So, I am fine marking it as "ready for committer". What do you say? ISTM that both of them are not against us, so let's ask for the committer's review! Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hi Ashutosh,
Thanks for the review!
(2014/11/28 18:14), Ashutosh Bapat wrote:
On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita
mailto:fujita.ets...@lab.ntt.co.jp>> wrote:
(2014/11/17 17:55), Ashutosh Bapat wrote:
Here are my review comments for patch fdw-inh-3.patch.
Tests
---
1. It seems like you have copied from testcase inherit.sql to
postgres_fdw testcase. That's a good thing, but it makes the
test quite
long. May be we should have two tests in postgres_fdw contrib
module,
one for simple cases, and other for inheritance. What do you say?
IMO, the test is not so time-consuming, so it doesn't seem worth
splitting it into two.
I am not worried about the timing but I am worried about the length of
the file and hence ease of debugging in case we find any issues there.
We will leave that for the commiter to decide.
OK
Documentation
1. The change in ddl.sgml
-We will refer to the child tables as partitions, though
they
-are in every way normal PostgreSQL tables.
+We will refer to the child tables as partitions, though
we assume
+that they are normal PostgreSQL tables.
adds phrase "we assume that", which confuses the intention
behind the
sentence. The original text is intended to highlight the equivalence
between "partition" and "normal table", where as the addition
esp. the
word "assume" weakens that equivalence. Instead now we have to
highlight
the equivalence between "partition" and "normal or foreign
table". The
wording similar to "though they are either normal or foreign tables"
should be used there.
You are right, but I feel that there is something out of place in
saying that there (5.10.2. Implementing Partitioning) because the
procedure there has been written based on normal tables only. Put
another way, if we say that, I think we'd need to add more docs,
describing the syntax and/or the corresponding examples for
foreign-table cases. But I'd like to leave that for another patch.
So, how about the wording "we assume *here* that", instead of "we
assume that", as I added the following notice in the previous
section (5.10.1. Overview)?
@@ -2650,7 +2669,10 @@ VALUES ('Albany', NULL, NULL, 'NY');
table of a single parent table. The parent table itself is
normally
empty; it exists just to represent the entire data set. You
should be
familiar with inheritance (see )
before
-attempting to set up partitioning.
+attempting to set up partitioning. (The setup and management of
+partitioned tables illustrated in this section assume that each
+partition is a normal table. However, you can do that in a
similar way
+for cases where some or all partitions are foreign tables.)
This looks ok, though, I would like to see final version of the
document. But I think, we will leave that for committer to handle.
OK
2. The wording "some kind of optimization" gives vague picture.
May be
it can be worded as "Since the constraints are assumed to be
true, they
are used in constraint-based query optimization like constraint
exclusion for partitioned tables.".
+Those constraints are used in some kind of query
optimization such
+as constraint exclusion for partitioned tables (see
+).
Will follow your revision.
Done.
Code
---
1. In the following change
+/*
* acquire_inherited_sample_rows -- acquire sample rows from
inheritance tree
*
* This has the same API as acquire_sample_rows, except that
rows are
* collected from all inheritance children as well as the
specified table.
- * We fail and return zero if there are no inheritance children.
+ * We fail and return zero if there are no inheritance children or
there are
+ * inheritance children that foreign tables.
The addition should be "there are inheritance children that *are all
*foreign tables. Note the addition "are all".
Sorry, I incorrectly wrote the comment. What I tried to write is
"We fail and return zero if there are no inheritance children or if
we are not in VAC_MODE_SINGLE case and inheritance tree contains at
least one foreign table.".
You might want to use "English" description of VAC_MODE_SINGLE instead
of that macro in the comment, so that reader d
Re: [HACKERS] inherit support for foreign tables
(2014/12/08 15:17), Ashutosh Bapat wrote: On Sat, Dec 6, 2014 at 9:21 PM, Noah Misch mailto:n...@leadboat.com>> wrote: Does this inheritance patch add any atomicity problem that goes away when one breaks up the inheritance hierarchy and UPDATEs each table separately? If not, this limitation is okay. If the UPDATES crafted after breaking up the inheritance hierarchy are needed to be run within the same transaction (as the UPDATE on inheritance hierarchy would do), yes, there is atomicity problem. ISTM that your concern would basically a known problem. Consider the following transaction. BEGIN TRANSACTION; UPDATE foo SET a = 100; -- updates on table foo in remote server1 UPDATE bar SET a = 100; -- updates on table bar in remote server2 COMMIT TRANSACTION; This transaction would cause the atomicity problem if pgfdw_xact_callback() for XACT_EVENT_PRE_COMMIT for foo succeeded and then that for bar failed during CommitTransaction(). Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/07 2:02), David Fetter wrote: On Thu, Dec 04, 2014 at 12:35:54PM +0900, Etsuro Fujita wrote: But I think there would be another idea. An example will be shown below. We show the update commands below the ModifyTable node, not above the corresponding ForeignScan nodes, so maybe less confusing. If there are no objections of you and others, I'll update the patch this way. postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) On public.ft1 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 ^^ It occurs to me that the command generated by the FDW might well not be SQL at all, as is the case with file_fdw and anything else that talks to a NoSQL engine. Would it be reasonable to call this "Remote command" or something similarly generic? Yeah, but I'd like to propose that this line is shown by the FDW API (ie, ExplainForeignModify) as in non-inherited update cases, so that the FDW developer can choose the right name. As for "On public.ft1", I'd like to propose that the FDW API also show that by calling a function for that introduced into the PG core (Would it be better to use "For" rather than "On"?). Sorry, my explanation was not enough. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/12/03 19:35), Ashutosh Bapat wrote: On Tue, Dec 2, 2014 at 8:29 AM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: This is not exactly extension of non-inheritance case. non-inheritance case doesn't show two remote SQLs under the same plan node. May be you can rename the label Remote SQL as Remote UPDATE/INSERT/DELETE (or something to that effect) for the DML command and the Foreign plan node should be renamed to Foreign access node or something to indicate that it does both the scan as well as DML. I am not keen about the actual terminology, but I think a reader of plan shouldn't get confused. We can leave this for committer's judgement. Thanks for the proposal! I think that would be a good idea. But I think there would be another idea. An example will be shown below. We show the update commands below the ModifyTable node, not above the corresponding ForeignScan nodes, so maybe less confusing. If there are no objections of you and others, I'll update the patch this way. postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) On public.ft1 Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 On public.ft2 Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) -> Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE -> Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. IIUC, while committing transactions involving a single remote server, the steps taken are as follows 1. the local changes are brought to PRE-COMMIT stage, which means that the transaction *will* succeed locally after successful completion of this phase, 2. COMMIT message is sent to the foreign server 3. If step two succeeds, local changes are committed and successful commit is conveyed to the client 4. if step two fails, local changes are rolled back and abort status is conveyed to the client 5. If step 1 itself fails, the remote changes are rolled back. This is as per one phase commit protocol which guarantees ACID for single foreign data source. So, the changes involving local and a single foreign server seem to be atomic and consistent. Really? Maybe I'm missing something, but I don't think the current implementation for committing transactions has such a mechanism stated in step 1. So, I think it's possible that the local transaction fails in step3 while the remote transaction succeeds, as mentioned above. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/28 18:14), Ashutosh Bapat wrote: On Thu, Nov 27, 2014 at 3:52 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: Apart from the above, I noticed that the patch doesn't consider to call ExplainForeignModify during EXPLAIN for an inherited UPDATE/DELETE, as shown below (note that there are no UPDATE remote queries displayed): So, I'd like to modify explain.c to show those queries like this: postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN --__--__- Update on public.parent (cost=0.00..280.77 rows=25 width=10) -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 -> Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 -> Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) Two "remote SQL" under a single node would be confusing. Also the node is labelled as "Foreign Scan". It would be confusing to show an "UPDATE" command under this "scan" node. I thought this as an extention of the existing (ie, non-inherited) case (see the below example) to the inherited case. postgres=# explain verbose update ft1 set a = a * 2 where a = 5; QUERY PLAN - Update on public.ft1 (cost=100.00..140.38 rows=12 width=10) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 -> Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (a * 2), ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE (5 rows) I think we should show update commands somewhere for the inherited case as for the non-inherited case. Alternatives to this are welcome. BTW, I was experimenting with DMLs being executed on multiple FDW server under same transaction and found that the transactions may not be atomic (and may be inconsistent), if one or more of the server fails to commit while rest of them commit the transaction. The reason for this is, we do not "rollback" the already "committed" changes to the foreign server, if one or more of them fail to "commit" a transaction. With foreign tables under inheritance hierarchy a single DML can span across multiple servers and the result may not be atomic (and may be inconsistent). So, IIUC, even the transactions over the local and the *single* remote server are not guaranteed to be executed atomically in the current form. It is possible that the remote transaction succeeds and the local one fails, for example, resulting in data inconsistency between the local and the remote. either we have to disable DMLs on an inheritance hierarchy which spans multiple servers. OR make sure that such transactions follow 2PC norms. -1 for disabling update queries on such an inheritance hierarchy because I think we should leave that to the user's judgment. And I think 2PC is definitely a separate patch. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
r possible future use. See the discussion in [1]. 4. In postgresGetForeignPlan(), the code added by this patch is required to handle the case when the row mark is placed on a parent table and hence is required to be applied on the child table. We need a comment explaining this. Otherwise, the three step process to get the row mark information isn't clear for a reader. Will add the comment. 5. In expand_inherited_rtentry() why do you need a separate variable hasForeign? Instead of using that variable, you can actually set/reset rte->hasForeign since existence of even a single foreign child would mark that member as true. - After typing this, I got the answer when I looked at the function code. Every child's RTE is initially a copy of parent's RTE and hence hasForeign status would be inherited by every child after the first foreign child. I think, this reasoning should be added as comment before assignment to rte->hasForeign at the end of the function. As you mentioned, I think we could set rte->hasForeign directly during the scan for the inheritance set, without the separate variable, hasForeign. But ISTM that it'd improve code readability to set rte->hasForeign using the separate variable at the end of the function because rte->hasForeign has its meaning only when rte->inh is true and because we know whether rte->inh is true, at the end of the function. 6. The tests in foreign_data.sql are pretty extensive. Thanks for that. I think, we should also check the effect of each of the following command using \d on appropriate tables. +CREATE FOREIGN TABLE ft2 () INHERITS (pt1) + SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value'); +ALTER FOREIGN TABLE ft2 NO INHERIT pt1; +DROP FOREIGN TABLE ft2; +CREATE FOREIGN TABLE ft2 ( + c1 integer NOT NULL, + c2 text, + c3 date +) SERVER s0 OPTIONS (delimiter ',', quote '"', "be quoted" 'value'); +ALTER FOREIGN TABLE ft2 INHERIT pt1; Will fix. Apart from the above, I noticed that the patch doesn't consider to call ExplainForeignModify during EXPLAIN for an inherited UPDATE/DELETE, as shown below (note that there are no UPDATE remote queries displayed): postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) -> Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE -> Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (10 rows) So, I'd like to modify explain.c to show those queries like this: postgres=# explain verbose update parent set a = a * 2 where a = 5; QUERY PLAN - Update on public.parent (cost=0.00..280.77 rows=25 width=10) -> Seq Scan on public.parent (cost=0.00..0.00 rows=1 width=10) Output: (parent.a * 2), parent.ctid Filter: (parent.a = 5) Remote SQL: UPDATE public.mytable_1 SET a = $2 WHERE ctid = $1 -> Foreign Scan on public.ft1 (cost=100.00..140.38 rows=12 width=10) Output: (ft1.a * 2), ft1.ctid Remote SQL: SELECT a, ctid FROM public.mytable_1 WHERE ((a = 5)) FOR UPDATE Remote SQL: UPDATE public.mytable_2 SET a = $2 WHERE ctid = $1 -> Foreign Scan on public.ft2 (cost=100.00..140.38 rows=12 width=10) Output: (ft2.a * 2), ft2.ctid Remote SQL: SELECT a, ctid FROM public.mytable_2 WHERE ((a = 5)) FOR UPDATE (12 rows) What do you say? Sorry for the delay. [1] http://www.postgresql.org/message-id/1316566782-sup-2...@alvh.no-ip.org Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/23 6:16), Tom Lane wrote: > I committed this with some cosmetic adjustments, and one not-so-cosmetic > one: Thanks for improving and committing the patch! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/19 18:21), Ashutosh Bapat wrote: Ok. I added that comment to the commitfest and changed the status to "ready for commiter". Many thanks! PS: the link to the review emails that discuss the issue doesn't work properly, so I re-added the link. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/19 15:56), Ashutosh Bapat wrote: On Wed, Nov 19, 2014 at 12:14 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: (2014/11/19 14:58), Ashutosh Bapat wrote: May be we should modify use_physical_tlist() to return false in case of RELKIND_FOREIGN_TABLE, so that we can use tlist in create_foreignscan_plan(). I do not see any create_*_plan() function using reltargetlist directly. Yeah, I think we can do that, but I'm not sure that we should use tlist in create_foreignscan_plan(), not rel->reltargetlist. How about leaving this for committers to decide. I am fine with that. May be you want to add an XXX comment there to bring it to the committer's notice. It's ok, but I'm not convinced with your idea. So, I think the comment can be adequately described by you, not by me. So, my proposal is for you to add the comment to the CF app. Could you do that? Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/19 14:55), Etsuro Fujita wrote: (2014/11/18 18:37), Ashutosh Bapat wrote: On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: (2014/11/17 19:54), Ashutosh Bapat wrote: Here are comments for postgres_fdw-syscol patch. 3. Since there is already a testcase which triggered this particular change, it will good, if we add that to regression in postgres_fdw. Done. I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE t1.tableoid = t2.oid. The condition makes sure that the tableoid in the row is same as the OID of the foreign table recorded in pg_class locally. And the EXPLAIN of the query which clearly shows that the tableoid column in not fetched from the foreign server. I thought that test, but I didn't use it because I think we can't see (at least from the EXPLAIN) why the qual is not pushed down: the qual isn't pushed down possibly becasue the qual is considered as a *join* qual, not because the qual just cotains tableoid. (Having said that, I think we can see if the qual isn't pushed down as a join qual for a parameterized plan, but ISTM it's worth complicating regression tests.) Sorry, I incorrectly wrote the last sentence. What I tried to write is, ISTM it's *not* worth complicating regression tests. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/19 14:58), Ashutosh Bapat wrote: On Wed, Nov 19, 2014 at 11:18 AM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: (2014/11/18 18:27), Ashutosh Bapat wrote: On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita Here are my comments about the patch fscan_reltargetlist.patch 2. Instead of using rel->reltargetlist, we should use the tlist passed by caller. This is the tlist expected from the Plan node. For foreign scans it will be same as rel->reltargetlist. Using tlist would make the function consistent with create_*scan_plan functions. I disagree with that for the reasons mentioned below: * For a foreign scan, tlist is *not* necessarily the same as rel->reltargetlist (ie, there is a case where tlist contains all user attributes while rel->reltargetlist contains only attributes actually needed by the query). In such a case it'd be inefficient to use tlist rather than rel->reltargetlist. create_foreignscan_plan() is called from create_scan_plan(), which passes the tlist. The later uses function use_physical_tlist() to decide which targetlist should be used (physical or actual). As per code below in this function 485 /* 486 * We can do this for real relation scans, subquery scans, function scans, 487 * values scans, and CTE scans (but not for, eg, joins). 488 */ 489 if (rel->rtekind != RTE_RELATION && 490 rel->rtekind != RTE_SUBQUERY && 491 rel->rtekind != RTE_FUNCTION && 492 rel->rtekind != RTE_VALUES && 493 rel->rtekind != RTE_CTE) 494 return false; 495 496 /* 497 * Can't do it with inheritance cases either (mainly because Append 498 * doesn't project). 499 */ 500 if (rel->reloptkind != RELOPT_BASEREL) 501 return false; For foreign tables as well as the tables under inheritance hierarchy it uses the actual targetlist, which contains only the required columns IOW rel->reltargetlist (see build_path_tlist()) with nested loop parameters substituted. So, it never included unnecessary columns in the targetlist. Maybe I'm missing something, but I think you are overlooking the case for foreign tables that are *not* under an inheritance hierarchy. (Note that the rtekind for foreign tables is RTE_RELATION.) Ah! you are right. I confused between rtekind and relkind. Sorry for that. May be we should modify use_physical_tlist() to return false in case of RELKIND_FOREIGN_TABLE, so that we can use tlist in create_foreignscan_plan(). I do not see any create_*_plan() function using reltargetlist directly. Yeah, I think we can do that, but I'm not sure that we should use tlist in create_foreignscan_plan(), not rel->reltargetlist. How about leaving this for committers to decide. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/18 18:37), Ashutosh Bapat wrote: On Tue, Nov 18, 2014 at 1:55 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: (2014/11/17 19:54), Ashutosh Bapat wrote: Here are comments for postgres_fdw-syscol patch. Code --- 1. Instead of a single liner comment "System columns can't be sent to remote.", it might be better to explain why system columns can't be sent to the remote. Done. I would add " and foreign values do not make sense locally (except may be ctid clubbed with foreign server_id)" to make it more clear. But I will leave that for the commiter to decide. OK. 3. Since there is already a testcase which triggered this particular change, it will good, if we add that to regression in postgres_fdw. Done. I think, a better case would be SELECT * FROM ft1 t1, pg_class t2 WHERE t1.tableoid = t2.oid. The condition makes sure that the tableoid in the row is same as the OID of the foreign table recorded in pg_class locally. And the EXPLAIN of the query which clearly shows that the tableoid column in not fetched from the foreign server. I thought that test, but I didn't use it because I think we can't see (at least from the EXPLAIN) why the qual is not pushed down: the qual isn't pushed down possibly becasue the qual is considered as a *join* qual, not because the qual just cotains tableoid. (Having said that, I think we can see if the qual isn't pushed down as a join qual for a parameterized plan, but ISTM it's worth complicating regression tests.) Once we resolve the other patch on this thread, I think this item can be marked as ready for commiter from my side. OK. Thank you for the review. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/18 18:27), Ashutosh Bapat wrote: On Tue, Nov 18, 2014 at 1:50 PM, Etsuro Fujita mailto:fujita.ets...@lab.ntt.co.jp>> wrote: (2014/11/17 19:36), Ashutosh Bapat wrote: Here are my comments about the patch fscan_reltargetlist.patch 2. Instead of using rel->reltargetlist, we should use the tlist passed by caller. This is the tlist expected from the Plan node. For foreign scans it will be same as rel->reltargetlist. Using tlist would make the function consistent with create_*scan_plan functions. I disagree with that for the reasons mentioned below: * For a foreign scan, tlist is *not* necessarily the same as rel->reltargetlist (ie, there is a case where tlist contains all user attributes while rel->reltargetlist contains only attributes actually needed by the query). In such a case it'd be inefficient to use tlist rather than rel->reltargetlist. create_foreignscan_plan() is called from create_scan_plan(), which passes the tlist. The later uses function use_physical_tlist() to decide which targetlist should be used (physical or actual). As per code below in this function 485 /* 486 * We can do this for real relation scans, subquery scans, function scans, 487 * values scans, and CTE scans (but not for, eg, joins). 488 */ 489 if (rel->rtekind != RTE_RELATION && 490 rel->rtekind != RTE_SUBQUERY && 491 rel->rtekind != RTE_FUNCTION && 492 rel->rtekind != RTE_VALUES && 493 rel->rtekind != RTE_CTE) 494 return false; 495 496 /* 497 * Can't do it with inheritance cases either (mainly because Append 498 * doesn't project). 499 */ 500 if (rel->reloptkind != RELOPT_BASEREL) 501 return false; For foreign tables as well as the tables under inheritance hierarchy it uses the actual targetlist, which contains only the required columns IOW rel->reltargetlist (see build_path_tlist()) with nested loop parameters substituted. So, it never included unnecessary columns in the targetlist. Maybe I'm missing something, but I think you are overlooking the case for foreign tables that are *not* under an inheritance hierarchy. (Note that the rtekind for foreign tables is RTE_RELATION.) Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/18 18:09), Ashutosh Bapat wrote: I looked at the patch. It looks good now. Once we have the inh patch ready, we can mark this item as ready for commiter. Thanks for the review! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/17 19:54), Ashutosh Bapat wrote:
Here are comments for postgres_fdw-syscol patch.
Thanks for the review!
Code
---
1. Instead of a single liner comment "System columns can't be sent to
remote.", it might be better to explain why system columns can't be sent
to the remote.
Done.
2. The comment in deparseVar is single line comment, so it should start
and end on the same line i.e. /* and */ should be on the same line.
Done.
3. Since there is already a testcase which triggered this particular
change, it will good, if we add that to regression in postgres_fdw.
Done.
Please find attached an updated version of the patch.
Thanks,
Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 253,258 foreign_expr_walker(Node *node,
--- 253,268
if (var->varno == glob_cxt->foreignrel->relid &&
var->varlevelsup == 0)
{
+ /*
+ * System columns can't be sent to remote since the values
+ * for system columns might be different between local and
+ * remote.
+ *
+ * XXX: we should probably send ctid to remote.
+ */
+ if (var->varattno < 0)
+ return false;
+
/* Var belongs to foreign table */
collation = var->varcollid;
state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***
*** 1262,1267 deparseVar(Var *node, deparse_expr_cxt *context)
--- 1272,1280
if (node->varno == context->foreignrel->relid &&
node->varlevelsup == 0)
{
+ /* Var shouldn't be a system column */
+ Assert(node->varattno >= 0);
+
/* Var belongs to foreign table */
deparseColumnRef(buf, node->varno, node->varattno, context->root);
}
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 353,358 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
--- 353,368
Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" = c2))
(3 rows)
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
+QUERY PLAN
+ -
+ Foreign Scan on public.ft1 t1
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+Filter: (t1.tableoid <> 0::oid)
+Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1"
+ (4 rows)
+
-- ===
-- WHERE with remotely-executable conditions
-- ===
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 180,185 EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = postgres_fdw_a
--- 180,187
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 === t1.c2;
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = abs(t1.c2);
EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.c1 = t1.c2;
+ -- where with system column conditions
+ EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 t1 WHERE t1.tableoid != 0;
-- ===
-- WHERE with remotely-executable conditions
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/17 19:36), Ashutosh Bapat wrote: Here are my comments about the patch fscan_reltargetlist.patch Thanks for the review! 1. This isn't your change, but we might be able to get rid of assignment 2071 /* Are any system columns requested from rel? */ 2072 scan_plan->fsSystemCol = false; since make_foreignscan() already does that. But I will leave upto you to remove this assignment or not. As you pointed out, the assignment is redundant, but I think that that'd improve the clarity and readability. So, I'd vote for leaving that as is. 2. Instead of using rel->reltargetlist, we should use the tlist passed by caller. This is the tlist expected from the Plan node. For foreign scans it will be same as rel->reltargetlist. Using tlist would make the function consistent with create_*scan_plan functions. I disagree with that for the reasons mentioned below: * For a foreign scan, tlist is *not* necessarily the same as rel->reltargetlist (ie, there is a case where tlist contains all user attributes while rel->reltargetlist contains only attributes actually needed by the query). In such a case it'd be inefficient to use tlist rather than rel->reltargetlist. * I think that it'd improve the readability to match the code with other places that execute similar processing, such as check_index_only() and remove_unused_subquery_outputs(). Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/12 20:04), Ashutosh Bapat wrote: I reviewed fdw-chk-3 patch. Here are my comments Thanks for the review! Tests --- 1. The tests added in file_fdw module look good. We should add tests for CREATE TABLE with CHECK CONSTRAINT also. Agreed. I added the tests, and also changed the proposed tests a bit. 2. For postgres_fdw we need tests to check the behaviour in case the constraints mismatch between the remote table and its local foreign table declaration in case of INSERT, UPDATE and SELECT. Done. 3. In the testcases for postgres_fdw it seems that you have forgotten to add statement after SET constraint_exclusion to 'partition' I added the statement. 4. In test foreign_data there are changes to fix the diffs caused by these changes like below ALTER FOREIGN TABLE ft1 DROP CONSTRAINT no_const; -- ERROR -ERROR: "ft1" is not a table +ERROR: constraint "no_const" of relation "ft1" does not exist ALTER FOREIGN TABLE ft1 DROP CONSTRAINT IF EXISTS no_const; -ERROR: "ft1" is not a table +NOTICE: constraint "no_const" of relation "ft1" does not exist, skipping ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check; -ERROR: "ft1" is not a table +ERROR: constraint "ft1_c1_check" of relation "ft1" does not exist Earlier when constraints were not supported for FOREIGN TABLE, these tests made sure the same functionality. So, even though the corresponding constraints were not created on the table (in fact it didn't allow the creation as well). Now that the constraints are allowed, I think the tests for "no_const" (without IF EXISTS) and "ft1_c1_check" are duplicating the same testcase. May be we should review this set of statement in the light of new functionality. Agreed. I removed the "DROP CONSTRAINT ft1_c1_check" test, and added a new test for ALTER CONSTRAINT. Code and implementation -- The usage of NO INHERIT and NOT VALID with CONSTRAINT on foreign table is blocked, but corresponding documentation entry doesn't mention so. Since foreign tables can not be inherited NO INHERIT option isn't applicable to foreign tables and the constraints on the foreign tables are declarative, hence NOT VALID option is also not applicable. So, I agree with what the code is doing, but that should be reflected in documentation with this explanation. Rest of the code modifies the condition checks for CHECK CONSTRAINTs on foreign tables, and it looks good to me. Done. Other changes: * Modified one error message that I added in AddRelationNewConstraints, to match the other message there. * Revised other docs a little bit. Attached is an updated version of the patch. Thanks, Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 62,68 CREATE FOREIGN TABLE tbl () SERVER file_server OPTIONS (format 'csv', null ' CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR CREATE FOREIGN TABLE agg_text ( ! a int2, b float4 ) SERVER file_server OPTIONS (format 'text', filename '@abs_srcdir@/data/agg.data', delimiter ' ', null '\N'); --- 62,68 CREATE FOREIGN TABLE tbl () SERVER file_server; -- ERROR CREATE FOREIGN TABLE agg_text ( ! a int2 CHECK (a >= 0), b float4 ) SERVER file_server OPTIONS (format 'text', filename '@abs_srcdir@/data/agg.data', delimiter ' ', null '\N'); *** *** 72,82 CREATE FOREIGN TABLE agg_csv ( --- 72,84 b float4 ) SERVER file_server OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.csv', header 'true', delimiter ';', quote '@', escape '"', null ''); + ALTER FOREIGN TABLE agg_csv ADD CHECK (a >= 0); CREATE FOREIGN TABLE agg_bad ( a int2, b float4 ) SERVER file_server OPTIONS (format 'csv', filename '@abs_srcdir@/data/agg.bad', header 'true', delimiter ';', quote '@', escape '"', null ''); + ALTER FOREIGN TABLE agg_bad ADD CHECK (a >= 0); -- per-column options tests CREATE FOREIGN TABLE text_csv ( *** *** 134,139 DELETE FROM agg_csv WHERE a = 100; --- 136,153 -- but this should be ignored SELECT * FROM agg_csv FOR UPDATE; + -- constraint exclusion tests + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + \t off + SELECT * FROM agg_csv WHERE a < 0; + SET constraint_exclusion = 'on'; + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + \t off + SELECT * FROM agg_csv WHERE a < 0; + SET constraint_exclusion = 'partition'; + -- privilege tests SET
Re: [HACKERS] Typos in CREATE TABLE doc
(2014/11/13 20:07), Heikki Linnakangas wrote:
> On 11/13/2014 12:45 PM, Etsuro Fujita wrote:
>> It seems to me there are typos in the reference page for CREATE TABLE.
>
> The structure of the sentence is a bit funky, but it seems correct to
> me. If you google for "should any", you'll get a bunch of pages
> discussing similar sentences.
>
> I would add a comma there, though: "Should any row of an insert or
> update operation produce a FALSE result, an exception is raised and ..."
I understand. So, Here is the comma patch.
Thanks,
Best regards,
Etsuro Fujita
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 299cce8..bc6db45 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -423,7 +423,7 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
Boolean result which new or updated rows must satisfy for an
insert or update operation to succeed. Expressions evaluating
to TRUE or UNKNOWN succeed. Should any row of an insert or
- update operation produce a FALSE result an error exception is
+ update operation produce a FALSE result, an error exception is
raised and the insert or update does not alter the database. A
check constraint specified as a column constraint should
reference that column's value only, while an expression
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typos in CREATE TABLE doc
It seems to me there are typos in the reference page for CREATE TABLE.
Please find attached a patch.
Thanks,
Best regards,
Etsuro Fujita
diff --git a/doc/src/sgml/ref/create_table.sgml b/doc/src/sgml/ref/create_table.sgml
index 299cce8..ebcb885 100644
--- a/doc/src/sgml/ref/create_table.sgml
+++ b/doc/src/sgml/ref/create_table.sgml
@@ -422,8 +422,8 @@ CREATE [ [ GLOBAL | LOCAL ] { TEMPORARY | TEMP } | UNLOGGED ] TABLE [ IF NOT EXI
The CHECK clause specifies an expression producing a
Boolean result which new or updated rows must satisfy for an
insert or update operation to succeed. Expressions evaluating
- to TRUE or UNKNOWN succeed. Should any row of an insert or
- update operation produce a FALSE result an error exception is
+ to TRUE or UNKNOWN succeed. If any row of an insert or
+ update operation produces a FALSE result, an error exception is
raised and the insert or update does not alter the database. A
check constraint specified as a column constraint should
reference that column's value only, while an expression
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
Hi Ashutosh, Thanks for the review! (2014/11/13 15:23), Ashutosh Bapat wrote: I tried to apply fdw-inh-3.patch on the latest head from master branch. It failed to apply using both patch and git apply. "patch" failed to apply because of rejections in contrib/file_fdw/output/file_fdw.source and doc/src/sgml/ref/create_foreign_table.sgml As I said upthread, fdw-inh-3.patch has been created on top of [1] and fdw-chk-3.patch. Did you apply these patche first? [1] https://commitfest.postgresql.org/action/patch_view?id=1599 Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
(2014/11/11 18:45), Etsuro Fujita wrote:
(2014/11/10 20:05), Ashutosh Bapat wrote:
Since two separate issues 1. using reltargetlist instead of attr_needed
and 2. system columns usage in FDW are being tackled here, we should
separate the patch into two one for each of the issues.
Agreed. Will split the patch into two.
Here are splitted patches:
fscan-reltargetlist.patch - patch for #1
postgres_fdw-syscol.patch - patch for #2
Thanks,
Best regards,
Etsuro Fujita
*** a/src/backend/optimizer/plan/createplan.c
--- b/src/backend/optimizer/plan/createplan.c
***
*** 20,25
--- 20,26
#include
#include "access/skey.h"
+ #include "access/sysattr.h"
#include "catalog/pg_class.h"
#include "foreign/fdwapi.h"
#include "miscadmin.h"
***
*** 1945,1950 create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
--- 1946,1953
RelOptInfo *rel = best_path->path.parent;
Index scan_relid = rel->relid;
RangeTblEntry *rte;
+ Bitmapset *attrs_used = NULL;
+ ListCell *lc;
int i;
/* it should be a base rel... */
***
*** 1993,2008 create_foreignscan_plan(PlannerInfo *root, ForeignPath *best_path,
* bit of a kluge and might go away someday, so we intentionally leave it
* out of the API presented to FDWs.
*/
scan_plan->fsSystemCol = false;
for (i = rel->min_attr; i < 0; i++)
{
! if (!bms_is_empty(rel->attr_needed[i - rel->min_attr]))
{
scan_plan->fsSystemCol = true;
break;
}
}
return scan_plan;
}
--- 1996,2030
* bit of a kluge and might go away someday, so we intentionally leave it
* out of the API presented to FDWs.
*/
+
+ /*
+ * Add all the attributes needed for joins or final output. Note: we must
+ * look at reltargetlist, not the attr_needed data, because attr_needed
+ * isn't computed for inheritance child rels.
+ */
+ pull_varattnos((Node *) rel->reltargetlist, rel->relid, &attrs_used);
+
+ /* Add all the attributes used by restriction clauses. */
+ foreach(lc, rel->baserestrictinfo)
+ {
+ RestrictInfo *rinfo = (RestrictInfo *) lfirst(lc);
+
+ pull_varattnos((Node *) rinfo->clause, rel->relid, &attrs_used);
+ }
+
+ /* Are any system columns requested from rel? */
scan_plan->fsSystemCol = false;
for (i = rel->min_attr; i < 0; i++)
{
! if (bms_is_member(i - FirstLowInvalidHeapAttributeNumber, attrs_used))
{
scan_plan->fsSystemCol = true;
break;
}
}
+ bms_free(attrs_used);
+
return scan_plan;
}
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 252,257 foreign_expr_walker(Node *node,
--- 252,265
if (var->varno == glob_cxt->foreignrel->relid &&
var->varlevelsup == 0)
{
+ /*
+ * System columns can't be sent to remote.
+ *
+ * XXX: we should probably send ctid to remote.
+ */
+ if (var->varattno < 0)
+ return false;
+
/* Var belongs to foreign table */
collation = var->varcollid;
state = OidIsValid(collation) ? FDW_COLLATE_SAFE : FDW_COLLATE_NONE;
***
*** 1261,1266 deparseVar(Var *node, deparse_expr_cxt *context)
--- 1269,1279
if (node->varno == context->foreignrel->relid &&
node->varlevelsup == 0)
{
+ /*
+ * System columns shouldn't be examined here.
+ */
+ Assert(node->varattno >= 0);
+
/* Var belongs to foreign table */
deparseColumnRef(buf, node->varno, node->varattno, context->root);
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/11/11 2:31), Fujii Masao wrote: On Mon, Nov 10, 2014 at 4:15 PM, Etsuro Fujita The patch looks good to me except for the following point: *** a/src/backend/access/gin/ginfast.c --- b/src/backend/access/gin/ginfast.c *** *** 25,30 --- 25,32 #include "utils/memutils.h" #include "utils/rel.h" + /* GUC parameter */ + int pending_list_cleanup_size = 0; I think we need to initialize the GUC to boot_val, 4096 in this case. No, IIUC basically the variable for GUC doesn't need to be initialized to its default value. OTOH, it's also harmless to initialize it to the default. I like the current code a bit because we don't need to change the initial value again when we decide to change the default value of GUC. I have no strong opinion about this, though. OK, so if there are no objections of others, I'll mark this as "Ready for Committer". Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] postgres_fdw behaves oddly
Hi Ashutosh, Thanks for the review! (2014/11/10 20:05), Ashutosh Bapat wrote: Since two separate issues 1. using reltargetlist instead of attr_needed and 2. system columns usage in FDW are being tackled here, we should separate the patch into two one for each of the issues. Agreed. Will split the patch into two. While I agree that the system columns shouldn't be sent to the remote node, it doesn't look clear to me as to what would they or their values mean in the context of foreign data. Some columns like tableoid would have a value which is the OID of the foreign table, other system columns like xmin/xmax/ctid will have different meanings (or no meaning) depending upon the foreign data source. In case of later columns, each FDW would have its own way of defining that meaning (I guess). But in any case, I agree that we shouldn't send the system columns to the remote side. Is there a way we can enforce this rule across all the FDWs? OR we want to tackle it separately per FDW? ISTM it'd be better to tackle it separately because I feel that such an enforcement by the PG core would go too far. I'm also concerned about a possibility of impedance mismatching between such an enforcement and postgres_fdw, which sends the ctid column to the remote side internally when executing UPDATE/DELETE on foreign tables. See postgresPlanForeignModify and eg, deparseUpdateSql. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/11/06 23:38), Fujii Masao wrote: On Tue, Nov 4, 2014 at 12:04 PM, Etsuro Fujita wrote: IIUC, I think that min = 0 disables fast update, so ISTM that it'd be appropriate to set min to some positive value. And ISTM that the idea of using the min value of work_mem is not so bad. OK. I changed the min value to 64kB. *** 356,361 CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ name + + + PENDING_LIST_CLEANUP_SIZE The above is still in uppercse. Fixed. Attached is the updated version of the patch. Thanks for the review! Thanks for the updating the patch! The patch looks good to me except for the following point: *** a/src/backend/access/gin/ginfast.c --- b/src/backend/access/gin/ginfast.c *** *** 25,30 --- 25,32 #include "utils/memutils.h" #include "utils/rel.h" + /* GUC parameter */ + int pending_list_cleanup_size = 0; I think we need to initialize the GUC to boot_val, 4096 in this case. I asked the opinions of others about the config_group of the GUC. But there seems no opinions, so I agree with Fujii-san's idea of assigning the GUC CLIENT_CONN_STATEMENT. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/11/07 14:57), Kyotaro HORIGUCHI wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I tried both patches on the current head, the newly added parameter to analyze_rel() hampered them from applying but it is easy to fix seemingly and almost all the other part was applied cleanly. Thanks for the review! By the way, are these the result of simply splitting of your last patch, foreign_inherit-v15.patch? http://www.postgresql.org/message-id/53feef94.6040...@lab.ntt.co.jp The answer is "no". The result of apllying whole-in-one version and this splitted version seem to have many differences. Did you added even other changes? Or do I understand this patch wrongly? As I said before, I splitted the whole-in-one version into three: 1) CHECK constraint patch (ie fdw-chk.patch), 2) table inheritance patch (ie fdw-inh.patch) and 3) path reparameterization patch (not posted). In addition to that, I slightly modified #1 and #2. IIUC, #3 would be useful not only for the inheritance cases but for union all cases. So, I plan to propose it independently in the next CF. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. I rebased the patches to the latest head. Here are updated patches. Other changes: * fdw-chk-3.patch: the updated patch revises some ereport messages a little bit. * fdw-inh-3.patch: I noticed that there is a doc bug in the previous patch. The updated patch fixes that, adds a bit more docs, and revises regression tests in foreign_data.sql a bit further. Thanks, Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 144,149 SET constraint_exclusion = 'partition'; --- 144,166 \t off ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- table inheritance tests + CREATE TABLE agg (a int2, b float4); + ALTER FOREIGN TABLE agg_csv INHERIT agg; + ALTER FOREIGN TABLE agg_text INHERIT agg; + SELECT * FROM agg WHERE b < 10.0 ORDER BY a, b; + SELECT * FROM ONLY agg WHERE b < 10.0 ORDER BY a, b; + SELECT * FROM agg_csv WHERE b < 10.0 ORDER BY a, b; + SELECT * FROM agg_text WHERE b < 10.0 ORDER BY a, b; + -- updates aren't supported + UPDATE agg SET a = 1; + DELETE FROM agg WHERE a = 100; + -- but this should be ignored + SELECT * FROM agg WHERE b < 10.0 ORDER BY a, b FOR UPDATE; + ALTER FOREIGN TABLE agg_csv NO INHERIT agg; + ALTER FOREIGN TABLE agg_text NO INHERIT agg; + DROP TABLE agg; + -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 237,242 EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; --- 237,289 SET constraint_exclusion = 'partition'; \t off ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- table inheritance tests + CREATE TABLE agg (a int2, b float4); + ALTER FOREIGN TABLE agg_csv INHERIT agg; + ALTER FOREIGN TABLE agg_text INHERIT agg; + SELECT * FROM agg WHERE b < 10.0 ORDER BY a, b; + a |b + +- + 0 | 0.09561 + 0 | 0.09561 + 56 | 7.8 + (3 rows) + + SELECT * FROM ONLY agg WHERE b < 10.0 ORDER BY a, b; + a | b + ---+--- + (0 rows) + + SELECT * FROM agg_csv WHERE b < 10.0 ORDER BY a, b; + a |b + ---+- + 0 | 0.09561 + (1 row) + + SELECT * FROM agg_text WHERE b < 10.0 ORDER BY a, b; + a |b + +- + 0 | 0.09561 + 56 | 7.8 + (2 rows) + + -- updates aren't supported + UPDATE agg SET a = 1; + ERROR: cannot update foreign table "agg_text" + DELETE FROM agg WHERE a = 100; + ERROR: cannot delete from foreign table "agg_text" + -- but this should be ignored + SELECT * FROM agg WHERE b < 10.0 ORDER BY a, b FOR UPDATE; + a |b + +- + 0 | 0.09561 + 0 | 0.09561 + 56 | 7.8 + (3 rows) + + ALTER FOREIGN TABLE agg_csv NO INHERIT agg; + ALTER FOREIGN TABLE agg_text NO INHERIT agg; + DROP TABLE agg; -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postg
Re: [HACKERS] inherit support for foreign tables
Hi Furuya-san, (2014/11/07 16:54), furu...@pm.nttdata.co.jp wrote: >> (2014/08/28 18:00), Etsuro Fujita wrote: >>> (2014/08/22 11:51), Noah Misch wrote: >>>> Today's ANALYZE VERBOSE messaging for former inheritance parents >>>> (tables with relhassubclass = true but no pg_inherits.inhparent >>>> links) is deceptive, and I welcome a fix to omit the spurious >>>> message. As defects go, this is quite minor. There's fundamentally >>>> no value in collecting inheritance tree statistics for such a parent, >>>> and no PostgreSQL command will do so. >> >>>> A >>>> credible alternative is to emit a second message indicating that we >>>> skipped the inheritance tree statistics after all, and why we skipped >>>> them. >> >>> I'd like to address this by emitting the second message as shown below: >> >>> A separate patch (analyze.patch) handles the former case in a similar >> way. >> >> I'll add to the upcoming CF, the analyze.patch as an independent item, >> which emits a second message indicating that we skipped the inheritance >> tree statistics and why we skipped them. > > I did a review of the patch. > There was no problem. > I confirmed the following. > > 1. applied cleanly and compilation was without warnings and errors > 2. all regress tests was passed ok > 3. The message output from ANALYZE VERBOSE. > > Following are the SQL which I used to check messages. > > create table parent (id serial); > create table child (name text) inherits (parent); > ANALYZE VERBOSE parent ; > drop table child ; > ANALYZE VERBOSE parent ; I think that that is a correct test for this patch. Thanks for the review! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Typo in comment
(2014/11/06 20:04), Fujii Masao wrote: On Thu, Nov 6, 2014 at 7:44 PM, Etsuro Fujita wrote: I ran into a typo in a comment in src/backend/commands/matview.c. Please find attached a patch. Thanks! Applied. Thanks! Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo in comment
I ran into a typo in a comment in src/backend/commands/matview.c. Please find attached a patch. Best regards, Etsuro Fujita diff --git a/src/backend/commands/matview.c b/src/backend/commands/matview.c index 30bd40d..db05f7c 100644 --- a/src/backend/commands/matview.c +++ b/src/backend/commands/matview.c @@ -473,7 +473,7 @@ transientrel_destroy(DestReceiver *self) * the given integer, to make a new table name based on the old one. * * This leaks memory through palloc(), which won't be cleaned up until the - * current memory memory context is freed. + * current memory context is freed. */ static char * make_temptable_name_n(char *tempname, int n) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/10/30 21:30), Fujii Masao wrote:
On Thu, Oct 30, 2014 at 7:30 PM, Etsuro Fujita
wrote:
Here are my review comments.
* The patch applies cleanly and make and make check run successfully. I
think that the patch is mostly good.
Thanks! Attached is the updated version of the patch.
Thank you for updating the patch!
* In src/backend/utils/misc/guc.c:
+ {
+ {"pending_list_cleanup_size", PGC_USERSET,
CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum size of the pending
list for GIN index."),
+NULL,
+ GUC_UNIT_KB
+ },
+ &pending_list_cleanup_size,
+ 4096, 0, MAX_KILOBYTES,
+ NULL, NULL, NULL
+ },
ISTM it'd be better to use RESOURCES_MEM, not CLIENT_CONN_STATEMENT. No?
Yes if the pending list always exists in the memory. But not, IIUC. Thought?
Exactly. But I think we can expect that in many cases, since I think
that the users would often set the GUC to a small value to the extent
that most of the pending list pages would be cached by shared buffer, to
maintain *search* performance.
I'd like to hear the opinions of others about the category for the GUC.
Also why not set min to 64, not to 0, in accoradance with that of work_mem?
I'm OK to use 64. But I just chose 0 because I could not think of any reasonable
reason why 64k is suitable as the minimum size of the pending list.
IOW, I have no idea about whether it's reasonable to use the min value of
work_mem as the min size of the pending list.
IIUC, I think that min = 0 disables fast update, so ISTM that it'd be
appropriate to set min to some positive value. And ISTM that the idea
of using the min value of work_mem is not so bad.
* In doc/src/sgml/ref/create_index.sgml:
+ PENDING_LIST_CLEANUP_SIZE
IMHO, it seems to me better for this variable to be in lowercase in
accordance with the GUC version.
Using lowercase only for pending_list_cleanup_size and uppercase for
other options
looks strange to me. What about using lowercase for all the storage options?
+1
I changed the document in that way.
*** 356,361 CREATE [ UNIQUE ] INDEX [ CONCURRENTLY ] [ class="parameter">name
--- 356,372
+
+
+ PENDING_LIST_CLEANUP_SIZE
The above is still in uppercse.
Thanks,
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/10/09 11:49), Etsuro Fujita wrote:
(2014/10/08 22:51), Fujii Masao wrote:
On Wed, Sep 24, 2014 at 11:10 AM, Etsuro Fujita
wrote:
On Wed, Sep 10, 2014 at 10:37 PM, Alvaro Herrera
wrote:
Fujii Masao wrote:
On Wed, Sep 10, 2014 at 12:15 PM, Etsuro Fujita
wrote:
PENDING_LIST_CLEANUP_SIZE and work_mem, for this setting.
Wouldn't it be easy-to-use to have only one parameter,
PENDING_LIST_CLEANUP_SIZE? How about setting
PENDING_LIST_CLEANUP_SIZE
to
work_mem as the default value when running the CREATE INDEX command?
So what about introduing pending_list_cleanup_size also as GUC?
That is, users can set the threshold by using either the reloption or
GUC.
Yes, I think having both a GUC and a reloption makes sense -- the GUC
applies to all indexes, and can be tweaked for individual indexes
using
the reloption.
OK, I'd vote for your idea of having both the GUC and the reloption.
So, I
think the patch needs to be updated. Fujii-san, what plan do you
have about
the patch?
Please see the attached patch. In this patch, I introduced the GUC
parameter,
pending_list_cleanup_size. I chose 4MB as the default value of the
parameter.
But do you have any better idea about that default value?
It seems reasonable to me that the GUC has the same default value as
work_mem. So, +1 for the default value of 4MB.
BTW, I moved the CommitFest entry of this patch to next CF 2014-10.
OK, I'll review the patch in the CF.
Thank you for updating the patch! Here are my review comments.
* The patch applies cleanly and make and make check run successfully. I
think that the patch is mostly good.
* In src/backend/utils/misc/guc.c:
+ {
+ {"pending_list_cleanup_size", PGC_USERSET,
CLIENT_CONN_STATEMENT,
+ gettext_noop("Sets the maximum size of the pending list for GIN
index."),
+NULL,
+ GUC_UNIT_KB
+ },
+ &pending_list_cleanup_size,
+ 4096, 0, MAX_KILOBYTES,
+ NULL, NULL, NULL
+ },
ISTM it'd be better to use RESOURCES_MEM, not CLIENT_CONN_STATEMENT. No?
Also why not set min to 64, not to 0, in accoradance with that of
work_mem?
* In src/backend/utils/misc/postgresql.conf.sample:
Likewise, why not put this variable in the section of RESOURCE USAGE,
not in that of CLIENT CONNECTION DEFAULTS.
* In src/backend/access/common/reloptions.c:
+ {
+ {
+ "pending_list_cleanup_size",
+ "Maximum size of the pending list for this GIN index, in
kilobytes.",
+ RELOPT_KIND_GIN
+ },
+ -1, 0, MAX_KILOBYTES
+ },
As in guc.c, why not set min to 64, not to 0?
* In src/include/access/gin.h:
/* GUC parameter */
extern PGDLLIMPORT int GinFuzzySearchLimit;
+ extern int pending_list_cleanup_size;
The comment should be "GUC parameters".
* In src/backend/access/gin/ginfast.c:
+ /* GUC parameter */
+ int pending_list_cleanup_size = 0;
Do we need to substitute 0 for pending_list_cleanup_size?
* In doc/src/sgml/config.sgml:
+ xreflabel="pending_list_cleanup_size">
+ pending_list_cleanup_size
(integer)
As in postgresql.conf.sample, ISTM it'd be better to explain this
variable in the section of Resource Consumption (maybe in "Memory"), not
in that of Client Connection Defaults.
* In doc/src/sgml/gin.sgml:
! pending_list_cleanup_size. To avoid fluctuations in
observed
ISTM it'd be better to use for pending_list_cleanup_size, not
, here.
* In doc/src/sgml/ref/create_index.sgml:
+ PENDING_LIST_CLEANUP_SIZE
IMHO, it seems to me better for this variable to be in lowercase in
accordance with the GUC version. Also, I think that the words "GIN
indexes accept a different parameter:" in the section of "Index Storage
Parameters" in this reference page would be "GIN indexes accept
different parameters:".
Sorry for the delay in reviewing the patch.
Best regards,
Etsuro Fujita
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Improve automatic analyze messages for inheritance trees
(2014/10/17 18:35), Etsuro Fujita wrote:
(2014/10/16 17:17), Simon Riggs wrote:
Would it be useful to keep track of how many tables just got analyzed?
i.e. analyze of foo (including N inheritance children)
I think that's a good idea. So, I'll update the patch.
Done. Attached is an updated version of the patch.
Thanks for the comment!
Best regards,
Etsuro Fujita
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
***
*** 648,659 do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
if (Log_autovacuum_min_duration == 0 ||
TimestampDifferenceExceeds(starttime, GetCurrentTimestamp(),
Log_autovacuum_min_duration))
! ereport(LOG,
! (errmsg("automatic analyze of table \"%s.%s.%s\" system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! pg_rusage_show(&ru0;
}
/* Roll back any GUC changes executed by index functions */
--- 648,699
if (Log_autovacuum_min_duration == 0 ||
TimestampDifferenceExceeds(starttime, GetCurrentTimestamp(),
Log_autovacuum_min_duration))
! {
! if (inh)
! {
! List *tableOIDs;
! int nchildren;
!
! /*
! * Find all members of inheritance set.
! *
! * Note: we already got the lock on children except for
! * temp tables of other backends. See
! * acquire_inherited_sample_rows.
! */
! tableOIDs =
! find_all_inheritors(RelationGetRelid(onerel), NoLock, NULL);
!
! /*
! * Compute the number of children.
! *
! * Note: this might contain temp tables of other backends,
! * which we didn't analyze, unless those tables have been
! * dropped concurrently. We could get the number of children
! * that we did analyze, if we were willing to change the API
! * of acquire_inherited_sample_rows to output it, or check
! * whether each child in tableOIDs is a temp table of another
! * backend or not using RELATION_IS_OTHER_TEMP(), but it
! * doesn't seem worth the code complexity or the overhead.
! */
! nchildren = list_length(tableOIDs) - 1;
!
! ereport(LOG,
! (errmsg("automatic analyze of table \"%s.%s.%s\" (including %d inheritance children) system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! nchildren,
! pg_rusage_show(&ru0;
! }
! else
! ereport(LOG,
! (errmsg("automatic analyze of table \"%s.%s.%s\" system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! pg_rusage_show(&ru0;
! }
}
/* Roll back any GUC changes executed by index functions */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/10/21 17:40), Etsuro Fujita wrote: (2014/10/14 20:00), Etsuro Fujita wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Done. And I've also a bit revised regression tests for both patches. Patches attached. Thanks, Best regards, Etsuro Fujita *** a/contrib/file_fdw/input/file_fdw.source --- b/contrib/file_fdw/input/file_fdw.source *** *** 134,139 DELETE FROM agg_csv WHERE a = 100; --- 134,149 -- but this should be ignored SELECT * FROM agg_csv FOR UPDATE; + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a >= 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; + -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/file_fdw/output/file_fdw.source --- b/contrib/file_fdw/output/file_fdw.source *** *** 219,224 SELECT * FROM agg_csv FOR UPDATE; --- 219,242 42 | 324.78 (3 rows) + -- constraint exclusion tests + ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a >= 0); + \t on + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + Foreign Scan on public.agg_csv +Output: a, b +Filter: (agg_csv.a < 0) +Foreign File: @abs_srcdir@/data/agg.csv + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0; + Result +Output: a, b +One-Time Filter: false + + SET constraint_exclusion = 'partition'; + \t off + ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check; -- privilege tests SET ROLE file_fdw_superuser; SELECT * FROM agg_text ORDER BY a; *** a/contrib/postgres_fdw/expected/postgres_fdw.out --- b/contrib/postgres_fdw/expected/postgres_fdw.out *** *** 2488,2493 select c2, count(*) from "S 1"."T 1" where c2 < 500 group by 1 order by 1; --- 2488,2515 (13 rows) -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 >= 0); + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 < 0; + QUERY PLAN + -- + Foreign Scan on public.ft1 +Output: c1, c2, c3, c4, c5, c6, c7, c8 +Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE ((c2 < 0)) + (3 rows) + + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 < 0; + QUERY PLAN + -- + Result +Output: c1, c2, c3, c4, c5, c6, c7, c8 +One-Time Filter: false + (3 rows) + + SET constraint_exclusion = 'partition'; + -- === -- test serial columns (ie, sequence-based defaults) -- === create table loc1 (f1 serial, f2 text); *** a/contrib/postgres_fdw/sql/postgres_fdw.sql --- b/contrib/postgres_fdw/sql/postgres_fdw.sql *** *** 387,392 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1; --- 387,401 select c2, count(*) from "S 1"."T 1" where c2 < 500 group by 1 order by 1; -- === + -- test constraint exclusion stuff + -- === + ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c2positive CHECK (c2 >= 0); + EXPLAIN (VERBOSE, COSTS false) SELECT * FROM ft1 WHERE c2 < 0; + SET constraint_exclusion = 'on'; + EXPLAIN (VERBOSE, COSTS false) SELECT * F
[HACKERS] Incorrect comment in tablecmds.c
I don't think that the lock level mentioned in the following comment in MergeAttributes() in tablecmds.c is right, since that that function has opened the relation with ShareUpdateExclusiveLock, not with AccessShareLock. Patch attached. 1749 /* 1750 * Close the parent rel, but keep our AccessShareLock on it until xact 1751 * commit. That will prevent someone else from deleting or ALTERing 1752 * the parent before the child is committed. 1753 */ 1754 heap_close(relation, NoLock); Thanks, Best regards, Etsuro Fujita *** a/src/backend/commands/tablecmds.c --- b/src/backend/commands/tablecmds.c *** *** 1747,1755 MergeAttributes(List *schema, List *supers, char relpersistence, pfree(newattno); /* ! * Close the parent rel, but keep our AccessShareLock on it until xact ! * commit. That will prevent someone else from deleting or ALTERing ! * the parent before the child is committed. */ heap_close(relation, NoLock); } --- 1747,1755 pfree(newattno); /* ! * Close the parent rel, but keep our ShareUpdateExclusiveLock on it ! * until xact commit. That will prevent someone else from deleting or ! * ALTERing the parent before the child is committed. */ heap_close(relation, NoLock); } -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/10/14 20:00), Etsuro Fujita wrote: Here are separated patches. fdw-chk.patch - CHECK constraints on foreign tables fdw-inh.patch - table inheritance with foreign tables The latter has been created on top of [1]. [1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp To be exact, it has been created on top of [1] and fdw-chk.patch. I noticed that the latter disallows TRUNCATE on inheritance trees that contain at least one child foreign table. But I think it would be better to allow it, with the semantics that we quietly ignore the child foreign tables and apply the operation to the child plain tables, which is the same semantics as ALTER COLUMN SET STORAGE on such inheritance trees. Comments welcome. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typos in comments
I ran into a type " a a " in a comment in snapmgr.c, and found that
there are four other places that've made the same typo, by the grep
command. And in one of those places, I found yet another typo. Please
find attached a patch.
Thanks,
Best regards,
Etsuro Fujita
diff --git a/src/backend/utils/adt/pgstatfuncs.c b/src/backend/utils/adt/pgstatfuncs.c
index 44ccd37..d621a68 100644
--- a/src/backend/utils/adt/pgstatfuncs.c
+++ b/src/backend/utils/adt/pgstatfuncs.c
@@ -1712,7 +1712,7 @@ pg_stat_reset_shared(PG_FUNCTION_ARGS)
PG_RETURN_VOID();
}
-/* Reset a a single counter in the current database */
+/* Reset a single counter in the current database */
Datum
pg_stat_reset_single_table_counters(PG_FUNCTION_ARGS)
{
diff --git a/src/backend/utils/adt/tsgistidx.c b/src/backend/utils/adt/tsgistidx.c
index bc45f11..8003918 100644
--- a/src/backend/utils/adt/tsgistidx.c
+++ b/src/backend/utils/adt/tsgistidx.c
@@ -306,7 +306,7 @@ checkcondition_arr(void *checkval, QueryOperand *val)
/* Loop invariant: StopLow <= val < StopHigh */
/*
- * we are not able to find a a prefix by hash value
+ * we are not able to find a prefix by hash value
*/
if (val->prefix)
return true;
@@ -329,7 +329,7 @@ static bool
checkcondition_bit(void *checkval, QueryOperand *val)
{
/*
- * we are not able to find a a prefix in signature tree
+ * we are not able to find a prefix in signature tree
*/
if (val->prefix)
return true;
diff --git a/src/backend/utils/adt/tsquery.c b/src/backend/utils/adt/tsquery.c
index b973a53..c8011a8 100644
--- a/src/backend/utils/adt/tsquery.c
+++ b/src/backend/utils/adt/tsquery.c
@@ -456,10 +456,10 @@ findoprnd(QueryItem *ptr, int size)
/*
- * Each value (operand) in the query is be passed to pushval. pushval can
+ * Each value (operand) in the query is passed to pushval. pushval can
* transform the simple value to an arbitrarily complex expression using
* pushValue and pushOperator. It must push a single value with pushValue,
- * a complete expression with all operands, or a a stopword placeholder
+ * a complete expression with all operands, or a stopword placeholder
* with pushStop, otherwise the prefix notation representation will be broken,
* having an operator with no operand.
*
diff --git a/src/backend/utils/time/snapmgr.c b/src/backend/utils/time/snapmgr.c
index 2834753..d601efe 100644
--- a/src/backend/utils/time/snapmgr.c
+++ b/src/backend/utils/time/snapmgr.c
@@ -283,7 +283,7 @@ GetNonHistoricCatalogSnapshot(Oid relid)
{
/*
* If the caller is trying to scan a relation that has no syscache, no
- * catcache invalidations will be sent when it is updated. For a a few
+ * catcache invalidations will be sent when it is updated. For a few
* key relations, snapshot invalidations are sent instead. If we're
* trying to scan a relation for which neither catcache nor snapshot
* invalidations are sent, we must refresh the snapshot every time.
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Improve automatic analyze messages for inheritance trees
(2014/10/16 17:17), Simon Riggs wrote: On 16 October 2014 06:49, Etsuro Fujita wrote: How about this? automatic analyze of table \"%s.%s.%s\" as inheritance tree Thank you for the comment. Would it be useful to keep track of how many tables just got analyzed? i.e. analyze of foo (including N inheritance children) I think that's a good idea. So, I'll update the patch. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/08/28 18:00), Etsuro Fujita wrote:
(2014/08/22 11:51), Noah Misch wrote:
Today's ANALYZE VERBOSE messaging for former inheritance parents
(tables with
relhassubclass = true but no pg_inherits.inhparent links) is
deceptive, and I
welcome a fix to omit the spurious message. As defects go, this is quite
minor. There's fundamentally no value in collecting inheritance tree
statistics for such a parent, and no PostgreSQL command will do so.
A
credible alternative is to emit a second message indicating that we
skipped
the inheritance tree statistics after all, and why we skipped them.
I'd like to address this by emitting the second message as shown below:
A separate patch (analyze.patch) handles the former case in a similar way.
I'll add to the upcoming CF, the analyze.patch as an independent item,
which emits a second message indicating that we skipped the inheritance
tree statistics and why we skipped them.
Thanks,
Best regards,
Etsuro Fujita
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
***
*** 1478,1483 acquire_inherited_sample_rows(Relation onerel, int elevel,
--- 1478,1487
/* CCI because we already updated the pg_class row in this command */
CommandCounterIncrement();
SetRelationHasSubclass(RelationGetRelid(onerel), false);
+ ereport(elevel,
+ (errmsg("skipping analyze of \"%s.%s\" inheritance tree --- this inheritance tree contains no child tables",
+ get_namespace_name(RelationGetNamespace(onerel)),
+ RelationGetRelationName(onerel;
return 0;
}
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Improve automatic analyze messages for inheritance trees
(2014/10/16 11:45), Simon Riggs wrote: On 6 October 2014 11:07, Etsuro Fujita wrote: I noticed that analyze messages shown by autovacuum don't discriminate between non-inherited cases and inherited cases, as shown in the below example: LOG: automatic analyze of table "postgres.public.pt" system usage: CPU 0.00s/0.01u sec elapsed 0.06 sec LOG: automatic analyze of table "postgres.public.pt" system usage: CPU 0.00s/0.02u sec elapsed 0.11 sec (The first one is for table "postgres.public.pt" and the second one is for table inheritance tree "postgres.public.pt".) So, I'd like to propose improving the messages for inherited cases, in order to easily distinguish such cases from non-inherited cases. Please find attached a patch. I'll add this to the upcoming CF. Thanks for the suggestion. It seems like a useful addition. Existing log analysis may wish to see the "automatic analyze of table" on each row. So it would be good to keep automatic analyze of table \"%s.%s.%s\" Agreed. Can we add some words after this to indicate inheritance? (I have no suggestions at present) e.g. automatic analyze of table \"%s.%s.%s\" (new words go here) How about this? automatic analyze of table \"%s.%s.%s\" as inheritance tree Thank you for the comment. Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] inherit support for foreign tables
(2014/09/12 16:30), Etsuro Fujita wrote:
(2014/09/11 20:51), Heikki Linnakangas wrote:
On 09/11/2014 02:30 PM, Etsuro Fujita wrote:
So, should I split the patch into the two?
Yeah, please do.
OK, Will do.
Here are separated patches.
fdw-chk.patch - CHECK constraints on foreign tables
fdw-inh.patch - table inheritance with foreign tables
The latter has been created on top of [1]. I've addressed the comment
from Horiguchi-san [2] in it also, though I've slightly modified his
proposal.
There is the functionality for path reparameterization for ForeignScan
in the previous patch, but since the functionality would be useful not
only for such table inheritance cases but for UNION ALL cases, I'd add
the functionality as an independent feature maybe to CF 2014-12.
Thanks,
[1] http://www.postgresql.org/message-id/540da168.3040...@lab.ntt.co.jp
[2]
http://www.postgresql.org/message-id/20140902.142218.253402812.horiguchi.kyot...@lab.ntt.co.jp
Best regards,
Etsuro Fujita
*** a/contrib/file_fdw/input/file_fdw.source
--- b/contrib/file_fdw/input/file_fdw.source
***
*** 134,139 DELETE FROM agg_csv WHERE a = 100;
--- 134,149
-- but this should be ignored
SELECT * FROM agg_csv FOR UPDATE;
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a >= 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
+
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/file_fdw/output/file_fdw.source
--- b/contrib/file_fdw/output/file_fdw.source
***
*** 219,224 SELECT * FROM agg_csv FOR UPDATE;
--- 219,242
42 | 324.78
(3 rows)
+ -- constraint exclusion tests
+ ALTER FOREIGN TABLE agg_csv ADD CONSTRAINT agg_csv_a_check CHECK (a >= 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ Foreign Scan on public.agg_csv
+Output: a, b
+Filter: (agg_csv.a < 0)
+Foreign File: @abs_srcdir@/data/agg.csv
+
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM agg_csv WHERE a < 0;
+ Result
+Output: a, b
+One-Time Filter: false
+
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE agg_csv DROP CONSTRAINT agg_csv_a_check;
-- privilege tests
SET ROLE file_fdw_superuser;
SELECT * FROM agg_text ORDER BY a;
*** a/contrib/postgres_fdw/expected/postgres_fdw.out
--- b/contrib/postgres_fdw/expected/postgres_fdw.out
***
*** 2488,2493 select c2, count(*) from "S 1"."T 1" where c2 < 500 group by 1 order by 1;
--- 2488,2512
(13 rows)
-- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1 > 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 <= 0;
+ Foreign Scan on public.ft1
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+Remote SQL: SELECT "C 1", c2, c3, c4, c5, c6, c7, c8 FROM "S 1"."T 1" WHERE (("C 1" <= 0))
+
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 <= 0;
+ Result
+Output: c1, c2, c3, c4, c5, c6, c7, c8
+One-Time Filter: false
+
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
+ -- ===
-- test serial columns (ie, sequence-based defaults)
-- ===
create table loc1 (f1 serial, f2 text);
*** a/contrib/postgres_fdw/sql/postgres_fdw.sql
--- b/contrib/postgres_fdw/sql/postgres_fdw.sql
***
*** 387,392 select c2, count(*) from ft2 where c2 < 500 group by 1 order by 1;
--- 387,404
select c2, count(*) from "S 1"."T 1" where c2 < 500 group by 1 order by 1;
-- ===
+ -- test constraint exclusion stuff
+ -- ===
+ ALTER FOREIGN TABLE ft1 ADD CONSTRAINT ft1_c1_check CHECK (c1 > 0);
+ \t on
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 <= 0;
+ SET constraint_exclusion = 'on';
+ EXPLAIN (VERBOSE, COSTS FALSE) SELECT * FROM ft1 WHERE c1 <= 0;
+ SET constraint_exclusion = 'partition';
+ \t off
+ ALTER FOREIGN TABLE ft1 DROP CONSTRAINT ft1_c1_check;
+
+ -- =
Re: [HACKERS] postgres_fdw behaves oddly
(2014/10/14 8:53), Robert Haas wrote: On Mon, Oct 13, 2014 at 3:30 PM, Bruce Momjian wrote: Uh, where are we on this patch? Probably it should be added to the upcoming CF. Done. https://commitfest.postgresql.org/action/patch_view?id=1599 Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/10/08 22:51), Fujii Masao wrote: On Wed, Sep 24, 2014 at 11:10 AM, Etsuro Fujita wrote: On Wed, Sep 10, 2014 at 10:37 PM, Alvaro Herrera wrote: Fujii Masao wrote: On Wed, Sep 10, 2014 at 12:15 PM, Etsuro Fujita wrote: PENDING_LIST_CLEANUP_SIZE and work_mem, for this setting. Wouldn't it be easy-to-use to have only one parameter, PENDING_LIST_CLEANUP_SIZE? How about setting PENDING_LIST_CLEANUP_SIZE to work_mem as the default value when running the CREATE INDEX command? So what about introduing pending_list_cleanup_size also as GUC? That is, users can set the threshold by using either the reloption or GUC. Yes, I think having both a GUC and a reloption makes sense -- the GUC applies to all indexes, and can be tweaked for individual indexes using the reloption. OK, I'd vote for your idea of having both the GUC and the reloption. So, I think the patch needs to be updated. Fujii-san, what plan do you have about the patch? Please see the attached patch. In this patch, I introduced the GUC parameter, pending_list_cleanup_size. I chose 4MB as the default value of the parameter. But do you have any better idea about that default value? It seems reasonable to me that the GUC has the same default value as work_mem. So, +1 for the default value of 4MB. BTW, I moved the CommitFest entry of this patch to next CF 2014-10. OK, I'll review the patch in the CF. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Improve automatic analyze messages for inheritance trees
I noticed that analyze messages shown by autovacuum don't discriminate
between non-inherited cases and inherited cases, as shown in the below
example:
LOG: automatic analyze of table "postgres.public.pt" system usage: CPU
0.00s/0.01u sec elapsed 0.06 sec
LOG: automatic analyze of table "postgres.public.pt" system usage: CPU
0.00s/0.02u sec elapsed 0.11 sec
(The first one is for table "postgres.public.pt" and the second one is
for table inheritance tree "postgres.public.pt".)
So, I'd like to propose improving the messages for inherited cases, in
order to easily distinguish such cases from non-inherited cases. Please
find attached a patch. I'll add this to the upcoming CF.
Thanks,
Best regards,
Etsuro Fujita
*** a/src/backend/commands/analyze.c
--- b/src/backend/commands/analyze.c
***
*** 648,659 do_analyze_rel(Relation onerel, VacuumStmt *vacstmt,
if (Log_autovacuum_min_duration == 0 ||
TimestampDifferenceExceeds(starttime, GetCurrentTimestamp(),
Log_autovacuum_min_duration))
! ereport(LOG,
! (errmsg("automatic analyze of table \"%s.%s.%s\" system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! pg_rusage_show(&ru0;
}
/* Roll back any GUC changes executed by index functions */
--- 648,669
if (Log_autovacuum_min_duration == 0 ||
TimestampDifferenceExceeds(starttime, GetCurrentTimestamp(),
Log_autovacuum_min_duration))
! {
! if (inh)
! ereport(LOG,
! (errmsg("automatic analyze of table inheritance tree \"%s.%s.%s\" system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! pg_rusage_show(&ru0;
! else
! ereport(LOG,
! (errmsg("automatic analyze of table \"%s.%s.%s\" system usage: %s",
! get_database_name(MyDatabaseId),
! get_namespace_name(RelationGetNamespace(onerel)),
! RelationGetRelationName(onerel),
! pg_rusage_show(&ru0;
! }
}
/* Roll back any GUC changes executed by index functions */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] Typo fixes in src/backend/rewrite/rewriteHandler.c
Here is the comments in process_matched_tle() in rewriteHandler.c. 883 * such nodes; consider 884 * UPDATE tab SET col.fld1.subfld1 = x, col.fld2.subfld2 = y 885 * The two expressions produced by the parser will look like 886 * FieldStore(col, fld1, FieldStore(placeholder, subfld1, x)) 887 * FieldStore(col, fld2, FieldStore(placeholder, subfld2, x)) I think the second one is not correct and should be FieldStore(col, fld2, FieldStore(placeholder, subfld2, y)) Just like this, 891 * FieldStore(FieldStore(col, fld1, 892 *FieldStore(placeholder, subfld1, x)), 893 * fld2, FieldStore(placeholder, subfld2, x)) should be FieldStore(FieldStore(col, fld1, FieldStore(placeholder, subfld1, x)), fld2, FieldStore(placeholder, subfld2, y)) Patch attached. Thanks, Best regards, Etsuro Fujita diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c index cb65c05..93fda07 100644 --- a/src/backend/rewrite/rewriteHandler.c +++ b/src/backend/rewrite/rewriteHandler.c @@ -883,13 +883,13 @@ process_matched_tle(TargetEntry *src_tle, * UPDATE tab SET col.fld1.subfld1 = x, col.fld2.subfld2 = y * The two expressions produced by the parser will look like * FieldStore(col, fld1, FieldStore(placeholder, subfld1, x)) - * FieldStore(col, fld2, FieldStore(placeholder, subfld2, x)) + * FieldStore(col, fld2, FieldStore(placeholder, subfld2, y)) * However, we can ignore the substructure and just consider the top * FieldStore or ArrayRef from each assignment, because it works to * combine these as * FieldStore(FieldStore(col, fld1, * FieldStore(placeholder, subfld1, x)), - * fld2, FieldStore(placeholder, subfld2, x)) + * fld2, FieldStore(placeholder, subfld2, y)) * Note the leftmost expression goes on the inside so that the * assignments appear to occur left-to-right. * -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] LIMIT for UPDATE and DELETE
(2014/09/17 1:58), Robert Haas wrote: On Tue, Sep 16, 2014 at 11:31 AM, Kevin Grittner wrote: Etsuro Fujita wrote: (2014/08/15 6:18), Rukh Meski wrote: Based on the feedback on my previous patch, I've separated only the LIMIT part into its own feature. This version plays nicely with inheritance. The intended use is splitting up big UPDATEs and DELETEs into batches more easily and efficiently. IIUC, the patch doesn't support OFFSET with UPDATE/DELETE ... LIMIT. Is that OK? When we support ORDER BY ... LIMIT/OFFSET, we will also be allowing for OFFSET with UPDATE/DELETE ... LIMIT. So, ISTM it would be better for the patch to support OFFSET at this point. No? Without ORDER BY you really would have no idea *which* rows the OFFSET would be skipping. Even more dangerously, you might *think* you do, and get a surprise when you see the results (if, for example, a seqscan starts at a point other than the start of the heap, due to a concurrent seqscan from an unrelated query). It might be better not to provide an illusion of a degree of control you don't have, especially for UPDATE and DELETE operations. Fair point, but I'd lean toward including it. I think we all agree the end goal is ORDER BY .. LIMIT, and there OFFSET certainly has meaning. If we don't include it now, we'll just end up adding it later. It makes for fewer patches, and fewer changes for users, if we do it all at once. I agree with Robert. Rukh, what do you think as an author? Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: PENDING_LIST_CLEANUP_SIZE - maximum size of GIN pending list Re: [HACKERS] HEAD seems to generate larger WAL regarding GIN index
(2014/09/13 2:42), Fujii Masao wrote: On Wed, Sep 10, 2014 at 10:37 PM, Alvaro Herrera wrote: Fujii Masao wrote: On Wed, Sep 10, 2014 at 12:15 PM, Etsuro Fujita wrote: PENDING_LIST_CLEANUP_SIZE and work_mem, for this setting. Wouldn't it be easy-to-use to have only one parameter, PENDING_LIST_CLEANUP_SIZE? How about setting PENDING_LIST_CLEANUP_SIZE to work_mem as the default value when running the CREATE INDEX command? That's an idea. But there might be some users who want to change the cleanup size per session like they can do by setting work_mem, and your idea would prevent them from doing that... So what about introduing pending_list_cleanup_size also as GUC? That is, users can set the threshold by using either the reloption or GUC. Yes, I think having both a GUC and a reloption makes sense -- the GUC applies to all indexes, and can be tweaked for individual indexes using the reloption. Agreed. I'm not sure about the idea of being able to change it per session, though. Do you mean that you would like insert processes use a very large value so that they can just append new values to the pending list, and have vacuum use a small value so that it cleans up as soon as it runs? Two things: 1. we could have an "autovacuum_" reloption which only changes what autovacuum does; 2. we could have autovacuum run index cleanup actions separately from actual vacuuming. Yes, I was thinking something like that. But if autovacuum has already been able to handle that, it's nice. Anyway, as you pointed out, it's better to have both GUC and reloption for the cleanup size of pending list. OK, I'd vote for your idea of having both the GUC and the reloption. So, I think the patch needs to be updated. Fujii-san, what plan do you have about the patch? Sorry for the delay. Thanks, Best regards, Etsuro Fujita -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
