Re: [HACKERS] Fixing matching of boolean index columns to sort ordering

[Michael Paquier]

On Wed, Dec 14, 2016 at 12:18 AM, David G. Johnston
 wrote:
> On Mon, Dec 12, 2016 at 10:08 PM, Tom Lane  wrote:
>>
>> Every WHERE clause is a
>>
>> boolean expression, so there's no principled reason why such a rule
>> wouldn't result in canonicalizing e.g. "i = 42" into "(i = 42) = true",
>> wreaking havoc on every other optimization we have.  Restricting it
>> to only apply to simple boolean columns is no answer because (a) indexes
>> can be on boolean expressions not just simple columns, and (b) part
>> of the point of the canonicalization is to make logically-equivalent
>> expressions look alike, so declining to apply it in some cases would
>> ruin that.
>
> Given our reliance on operators in indexing a canonicalization rule that
> says all boolean yielding expressions must contain an operator would yield
> the desired result.  "i = 42" already has an operator so no further
> normalization is necessary to make it conform to such a rule.
>
> The indexing concern may still come into play here, I'm not familiar enough
> with indexes on column lists versus indexes on expressions to know off the
> top of my head.  The definition of "looking alike" seems like it would be
> met since all such expression would look alike in having an operator.
>
> That said, not adding "= true" is more visually appealing - though given all
> of the other things we do in ruleutils this seems like a minor addition.

I have spent a couple of hours eye-balling this patch. There are not
that many users with indexes including a boolean column in its
definition... Still as this patch pushes down an index scan and avoids
an order by relying on the index scan to get things in the right order
it looks definitely right to make things better if we can. So that
seems worth it to me, even if that's adding a new extra
boolean-related optimization.

And actually, contrary to what is mentioned upthread, the planner is
not able to avoid a sort phase if other data types are used, say:
=# create table foo (a int, b int);
CREATE TABLE
=# create index on foo (a, b);
CREATE INDEX
=# explain (costs off) select * from foo where a = 1 order by b limit 10;
 QUERY PLAN

 Limit
   ->  Sort
 Sort Key: b
 ->  Bitmap Heap Scan on foo
   Recheck Cond: (a = 1)
   ->  Bitmap Index Scan on foo_a_b_idx
 Index Cond: (a = 1)
(7 rows)
In this case, the sorting on column b should not be necessary as it
could be possible to rely on the index scan to get the elements
already sorted. Maybe I am missing something?
-- 
Michael


-- 
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] Push down more full joins in postgres_fdw

[Etsuro Fujita]

On 2017/01/12 18:25, Ashutosh Bapat wrote:

On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita
 wrote:



On 2017/01/05 21:11, Ashutosh Bapat wrote:

IIUC, for a relation with use_remote_estimates we will deparse the
query twice and will build the targetlist twice.



While working on this, I noticed a weird case.  Consider:

postgres=# explain verbose select 1 from ft1 left join ft2 on (ft1.a =
ft2.a) inner join test on (true);
   QUERY PLAN
-
 Nested Loop  (cost=100.00..103.06 rows=1 width=4)
   Output: 1
   ->  Foreign Scan  (cost=100.00..102.04 rows=1 width=0)
 Relations: (public.ft1) LEFT JOIN (public.ft2)
 Remote SQL: SELECT NULL FROM (public.t1 r1 LEFT JOIN public.t2 r2
ON (((r1.a = r2.a
   ->  Seq Scan on public.test  (cost=0.00..1.01 rows=1 width=0)
 Output: test.a, test.b
(7 rows)

In this case the fpinfo->tlist of the foreign join is NIL, so whether or not
the tlist is already built cannot be discriminated by the fpinfo->tlist.  We
might need another flag to show that the tlist has been built already.
Since this is an existing issue and we would need to give careful thought to
this, so I'd like to leave this for another patch.



I think in that case, relation's targetlist will also be NIL or
contain no Var node. It wouldn't be expensive to build it again and
again. That's better than maintaining a new flag.


I think that's ugly.  A more clean way I'm thinking is: (1) in 
postgresGetForeignJoinPaths(), create a tlist by 
build_tlist_to_deparse() and save it in fpinfo->tlist before 
estimate_path_cost_size() if use_remote_estimates=true, (2) in 
estimate_path_cost_size(), use the fpinfo->tlist if 
use_remote_estimates=true, and (3) in postgresGetForeignPlan(), use the 
fpinfo->tlist as the fdw_scan_tlist if use_remote_estimates=true, 
otherwise create a tlist as the fdw_scan_tlist by 
build_tlist_to_deparse(), like the attached.


What do you think about that?

Another change is: I simplified build_tlist_to_deparse() a bit and put 
that in postgres_fdw.c because that is used only in postgres_fdw.c.


I still think we should discuss this separately because this is an 
existing issue and that makes it easy to review the patch.  If the 
attached is the right way to go, I'll update the join-pushdown patch on 
top of the patch.


Best regards,
Etsuro Fujita
*** a/contrib/postgres_fdw/deparse.c
--- b/contrib/postgres_fdw/deparse.c
***
*** 843,883  deparse_type_name(Oid type_oid, int32 typemod)
  }
  
  /*
-  * Build the targetlist for given relation to be deparsed as SELECT clause.
-  *
-  * The output targetlist contains the columns that need to be fetched from the
-  * foreign server for the given relation.  If foreignrel is an upper relation,
-  * then the output targetlist can also contain expressions to be evaluated on
-  * foreign server.
-  */
- List *
- build_tlist_to_deparse(RelOptInfo *foreignrel)
- {
- 	List	   *tlist = NIL;
- 	PgFdwRelationInfo *fpinfo = (PgFdwRelationInfo *) foreignrel->fdw_private;
- 
- 	/*
- 	 * For an upper relation, we have already built the target list while
- 	 * checking shippability, so just return that.
- 	 */
- 	if (foreignrel->reloptkind == RELOPT_UPPER_REL)
- 		return fpinfo->grouped_tlist;
- 
- 	/*
- 	 * We require columns specified in foreignrel->reltarget->exprs and those
- 	 * required for evaluating the local conditions.
- 	 */
- 	tlist = add_to_flat_tlist(tlist,
- 	   pull_var_clause((Node *) foreignrel->reltarget->exprs,
- 	   PVC_RECURSE_PLACEHOLDERS));
- 	tlist = add_to_flat_tlist(tlist,
- 			  pull_var_clause((Node *) fpinfo->local_conds,
- 			  PVC_RECURSE_PLACEHOLDERS));
- 
- 	return tlist;
- }
- 
- /*
   * Deparse SELECT statement for given relation into buf.
   *
   * tlist contains the list of desired columns to be fetched from foreign server.
--- 843,848 
*** a/contrib/postgres_fdw/postgres_fdw.c
--- b/contrib/postgres_fdw/postgres_fdw.c
***
*** 406,411  static void conversion_error_callback(void *arg);
--- 406,412 
  static bool foreign_join_ok(PlannerInfo *root, RelOptInfo *joinrel,
  JoinType jointype, RelOptInfo *outerrel, RelOptInfo *innerrel,
  JoinPathExtraData *extra);
+ static List *build_tlist_to_deparse(RelOptInfo *foreignrel);
  static bool foreign_grouping_ok(PlannerInfo *root, RelOptInfo *grouped_rel);
  static List *get_useful_pathkeys_for_relation(PlannerInfo *root,
   RelOptInfo *rel);
***
*** 1190,1197  postgresGetForeignPlan(PlannerInfo *root,
  		remote_conds = fpinfo->remote_conds;
  		local_exprs = fpinfo->local_conds;
  
! 		/* Build the list of columns to be fetched from the foreign server. */
! 		fdw_scan_tlist = build_tlist_to_deparse(foreignrel);
  
  		/*
  		 * Ensure that the outer plan produces a tuple whose descriptor
--- 1191,1209 

Re: [HACKERS] Misplacement of function declaration in contrib/postgres_fdw/postgres_fdw.h

[Etsuro Fujita]

On 2017/01/12 13:52, Ashutosh Bapat wrote:

On Thu, Jan 12, 2017 at 8:49 AM, Etsuro Fujita
 wrote:

While working on pushing down more joins/updates to the remote, I noticed
that in contrib/postgres_fdw/postgres_fdw.h the declaration of
get_jointype_name is misplaced in the section of shippable.c.  Since that
function is defined in contrib/postgres_fdw/deparse.c, we should put that
declaration in the section of deparse.c in the header file. Attached is a
patch for that.



I think, initially (probably in a never committed patch) the function
was used to check whether a join type is shippable and if so return
the name to be used in the query. That may be the reason why it ended
up in shippability.c. But later the shippability test was separated
from the code which required the string representation.


Thanks for the explanation!


Thanks for
pointing out the descripancy. The patch looks good. As a side change,
should we include "JOIN" in the string returned by this fuction? The
two places where this function is called, append "JOIN" to the string
returned by this function.


I was thinking that, so +1.


Although, even without that change, the
patch looks good.


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] Transactions involving multiple postgres foreign servers

[Masahiko Sawada]

On Fri, Jan 13, 2017 at 3:20 PM, Ashutosh Bapat
 wrote:
>>>
>>
>> Long time passed since original patch proposed by Ashutosh, so I
>> explain again about current design and functionality of this feature.
>> If you have any question, please feel free to ask.
>
> Thanks for the summary.
>
>>
>> Parameters
>> ==
>
> [ snip ]
>
>>
>> Cluster-wide atomic commit
>> ===
>> Since the distributed transaction commit on foreign servers are
>> executed independently, the transaction that modified data on the
>> multiple foreign servers is not ensured that transaction did either
>> all of them commit or all of them rollback. The patch adds the
>> functionality that guarantees distributed transaction did either
>> commit or rollback on all foreign servers. IOW the goal of this patch
>> is achieving the cluster-wide atomic commit across foreign server that
>> is capable two phase commit protocol.
>
> In [1], I proposed that we solve the problem of supporting PREPARED
> transactions involving foreign servers and in subsequent mail Vinayak
> agreed to that. But this goal has wider scope than that proposal. I am
> fine widening the scope, but then it would again lead to the same
> discussion we had about the big picture. May be you want to share
> design (or point out the parts of this design that will help) for
> solving smaller problem and tone down the patch for the same.
>

Sorry for confuse you. I'm still focusing on solving only that
problem. What I was trying to say is that I think that supporting
PREPARED transaction involving foreign server is the means, not the
end. So once we supports PREPARED transaction involving foreign
servers we can achieve cluster-wide atomic commit in a sense.

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] plpgsql - additional extra checks

[Pavel Stehule]

2017-01-13 2:46 GMT+01:00 Jim Nasby :

> On 1/11/17 5:54 AM, Pavel Stehule wrote:
>
>> +too_many_rows
>> +
>> + 
>> +  When result is assigned to a variable by INTO
>> clause,
>> +  checks if query returns more than one row. In this case the
>> assignment
>> +  is not deterministic usually - and it can be signal some issues in
>> design.
>>
>
> Shouldn't this also apply to
>
> var := blah FROM some_table WHERE ...;
>

yes, it is possible.

I am not sure - how to document it. This pattern is undocumented and it is
side effect of our parser.

If somebody use var := (SELECT xxx FROM ..)

what is correct syntax, then exactly only one row is required.

But the extra check is ok, if we technically allows this syntax.

Regards

Pavel


> ?
>
> AIUI that's one of the beefs the plpgsql2 project has.
>
> FWIW, I'd also be in favor of a NOMULTI option to INTO, but I don't see
> any way to do something like that with var := blah FROM.
> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>

Re: [HACKERS] Unused member root in foreign_glob_cxt

[Ashutosh Bapat]

On Thu, Jan 12, 2017 at 6:39 PM, Tom Lane  wrote:
> Ashutosh Bapat  writes:
>> The member root in foreign_glob_cxt isn't used anywhere by
>> postgres_fdw code. Without that member the code compiles and
>> regression passes. The member was added by d0d75c40. I looked at that
>> commit briefly but did not find any code using it there. So, possibly
>> it's unused since it was introduced. Should we drop that member?
>
> I think you'd just end up putting it back at some point.  It's the only
> means that foreign_expr_walker() has for getting at the root pointer,
> and nearly all planner code needs that.  Seems to me it's just chance
> that foreign_expr_walker() doesn't need it right now.

I agree with you that most of the planner code needs that but not
every planner function accepts root as an argument. The commit was 4
years before and since then we have added support for Analyze, WHERE,
join, aggregate, DML pushdown. None of them required it and it's
likely that none of the future work would require it as we have
covered almost all kinds of expressions in there.

>
>> PFA the patch to remove that member. If we decide to drop that member,
>> we can drop root argument to is_foreign_expr() and clean up some more
>> code. I volunteer to do that, if we agree.
>
> That would *really* be doubling down on the assumption that
> is_foreign_expr() will never ever need access to the root.
>

I think the cleanup won't expand beyond is_foreign_expr() itself.
Almost all of its callers, use root for some other reason. We will add
root to is_foreign_expr() when we need it, but that time we will know
"why" it's there.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] Transactions involving multiple postgres foreign servers

[Ashutosh Bapat]

>>
>
> Long time passed since original patch proposed by Ashutosh, so I
> explain again about current design and functionality of this feature.
> If you have any question, please feel free to ask.

Thanks for the summary.

>
> Parameters
> ==

[ snip ]

>
> Cluster-wide atomic commit
> ===
> Since the distributed transaction commit on foreign servers are
> executed independently, the transaction that modified data on the
> multiple foreign servers is not ensured that transaction did either
> all of them commit or all of them rollback. The patch adds the
> functionality that guarantees distributed transaction did either
> commit or rollback on all foreign servers. IOW the goal of this patch
> is achieving the cluster-wide atomic commit across foreign server that
> is capable two phase commit protocol.

In [1], I proposed that we solve the problem of supporting PREPARED
transactions involving foreign servers and in subsequent mail Vinayak
agreed to that. But this goal has wider scope than that proposal. I am
fine widening the scope, but then it would again lead to the same
discussion we had about the big picture. May be you want to share
design (or point out the parts of this design that will help) for
solving smaller problem and tone down the patch for the same.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
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] Packages: Again

[Pavel Stehule]

2017-01-13 3:07 GMT+01:00 Joshua D. Drake :

> On 01/12/2017 03:35 PM, Craig Ringer wrote:
>
>>
>>
>
>> So far that's all "that'd be nice, but isn't a technical barrier" stuff.
>>
>> Package variables for example. When _do_ you _need_ them? For what? (I'm
>> aware of some uses but "when you need them" helps us not at all).
>>
>>
> Well my answer would be, "because we want Oracle people to have an easy
> time migrating". I know that isn't the -hackers answer but clearly there is
> a demand for them. I would note that EDB Advanced server supports them,
> exactly because there is a demand for them.
>

EDB try to emulate Oracle - so some special features are necessary there -
although are redundant to existing PostgreSQL features - packages,
collections, ..


>
> Again, I am not making the technical argument here. I don't have the time
> to research it. I am making a usability argument for a potentially huge
> portion of database users to allow PostgreSQL to be more attractive to
> them, argument.
>
> I also received this today:
>
> """
> Well, packages make programming much easier. Not only do you keep related
> procedures together, you can also have private package variables and the
> package initialization. Also, packages are units of security, so you can
> grant permissions on the package to the entire group of users and if you
> later modify the package and add a function, you don't need to grant it
> separately.
> """
>

This is possible with our schemas too. I use schemas for package emulation
for Orafce 10 years, and it is working well.

The main problem is in different languages - our "database" <> Oracle
"database", our "schema" <> Oracle "schema"

People doesn't want packages - they want Oracle without any fee


> Sincerely,
>
> JD
>
>
>
>
> --
> Command Prompt, Inc.  http://the.postgres.company/
> +1-503-667-4564
> PostgreSQL Centered full stack support, consulting and development.
> Everyone appreciates your honesty, until you are honest with them.
> Unless otherwise stated, opinions are my own.
>
>
> --
> 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] pg_background contrib module proposal

[amul sul]

On Tue, Jan 10, 2017 at 7:09 AM, Jim Nasby  wrote:
> On 1/9/17 7:22 AM, amul sul wrote:
>>
>> On Sun, Jan 8, 2017 at 8:50 AM, Jim Nasby 
>> wrote:
>>>
>>>
>> [skipped...]
>>>
>>>
>>> Oh, hmm. So I guess if you do that when the background process is idle
>>> it's
>>> the same as a close?
>>>
>>> I think we need some way to safeguard against accidental forkbombs for
>>> cases
>>> where users aren't intending to leave something running in the
>>> background.
>>> There's other reasons to use this besides spawning long running
>>> processes,
>>> and I'd certainly want to be able to ensure the calling function wasn't
>>> accidentally leaving things running that it didn't mean to. (Maybe the
>>> patch
>>> already does this...)
>>>
>>
>> Current pg_background patch built to the top of BackgroundSession code
>> take care of that;
>> user need to call pg_background_close() to gracefully close previously
>> forked background
>> worker.  Even though if user session who forked this worker exited
>> without calling
>> pg_background_close(), this background worked force to exit with following
>> log:
>>
>> ERROR:  could not read from message queue: Other process has detached
>> queue
>> LOG:  could not send on message queue: Other process has detached queue
>> LOG:  worker process: background session by PID 61242 (PID 61358)
>> exited with exit code 1
>>
>> Does this make sense to you?
>
>
> I'm specifically thinking of autonomous transactions, where you do NOT want
> to run the command in the background, but you do want to run it in another
> connection.
>
> The other example is running a command in another database. Not the same as
> the autonomous transaction use case, but once again you likely don't want
> that command running in the background.
>
> Put another way, when you launch a new process in a shell script, you have
> to do something specific for that process to run in the background.
>
> My concern here is that AIUI the only way to launch a bgworker is with it
> already backgrounded. That makes it much more likely that a bug in your code
> produces an unintended fork-bomb (connection-bomb?). That would be
> especially true if the function was re-entrant.
>
> Since one of the major points of bgworkers is exactly to run stuff in the
> background, while the parent connection is doing something else, I can
> certainly understand the default case being to background something, but I
> think it'd be good to have a simple way to start a bgworker, have it do
> something, and wait until it's done.
>
> Make sense?
>
I am not sure that I've understand this completely, but note that in
current pg_background design we simply launch worker once and feed the
subsequent queries using pg_background_run() api.

Regards,
Amul


-- 
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] Gather Merge

[Rushabh Lathia]

On Thu, Jan 12, 2017 at 8:50 AM, Robert Haas  wrote:

> On Sun, Dec 4, 2016 at 7:36 PM, Haribabu Kommi 
> wrote:
> > On Thu, Nov 24, 2016 at 11:12 PM, Rushabh Lathia <
> rushabh.lat...@gmail.com>
> > wrote:
> >> PFA latest patch with fix as well as few cosmetic changes.
> >
> > Moved to next CF with "needs review" status.
>
> I spent quite a bit of time on this patch over the last couple of
> days.  I was hoping to commit it, but I think it's not quite ready for
> that yet and I hit a few other issues along the way.  Meanwhile,
> here's an updated version with the following changes:
>
> * Adjusted cost_gather_merge because we don't need to worry about less
> than 1 worker.
> * Don't charge double maintenance cost of the heap per 34ca0905.  This
> was pointed out previous and Rushabh said it was fixed, but it wasn't
> fixed in v5.
> * cost_gather_merge claimed to charge a slightly higher IPC cost
> because we have to block, but didn't.  Fix it so it does.
> * Move several hunks to more appropriate places in the file, near
> related code or in a more logical position relative to surrounding
> code.
> * Fixed copyright dates for the new files.  One said 2015, one said 2016.
> * Removed unnecessary code from create_gather_merge_plan that tried to
> handle an empty list of pathkeys (shouldn't happen).
> * Make create_gather_merge_plan more consistent with
> create_merge_append_plan.  Remove make_gather_merge for the same
> reason.
> * Changed generate_gather_paths to generate gather merge paths.  In
> the previous coding, only the upper planner nodes ever tried to
> generate gather merge nodes, but that seems unnecessarily limiting,
> since it could be useful to generate a gathered path with pathkeys at
> any point in the tree where we'd generate a gathered path with no
> pathkeys.
> * Rewrote generate_ordered_paths() logic to consider only the one
> potentially-useful path not now covered by the new code in
> generate_gather_paths().
> * Reverted changes in generate_distinct_paths().  I think we should
> add something here but the existing logic definitely isn't right
> considering the change to generate_gather_paths().
> * Assorted cosmetic cleanup in nodeGatherMerge.c.
> * Documented the new GUC enable_gathermerge.
> * Improved comments.  Dropped one that seemed unnecessary.
> * Fixed parts of the patch to be more pgindent-clean.
>
>
Thanks Robert for hacking into this.


> Testing this against the TPC-H queries at 10GB with
> max_parallel_workers_per_gather = 4, seq_page_cost = 0.1,
> random_page_cost = 0.1, work_mem = 64MB initially produced somewhat
> demoralizing results.  Only Q17, Q4, and Q8 picked Gather Merge, and
> of those only Q17 got faster.  Investigating this led to me realizing
> that join costing for parallel joins is all messed up: see
> https://www.postgresql.org/message-id/CA+TgmoYt2pyk2CTyvYCtFySXN=
> jsorgh8_mjttlowu5qkjo...@mail.gmail.com
>
> With that patch applied, in my testing, Gather Merge got picked for
> Q3, Q4, Q5, Q6, Q7, Q8, Q10, and Q17, but a lot of those queries get a
> little slower instead of a little faster.  Here are the timings --
> these are with EXPLAIN ANALYZE, so take them with a grain of salt --
> first number is without Gather Merge, second is with Gather Merge:
>
> Q3 16943.938 ms -> 18645.957 ms
> Q4 3155.350 ms -> 4179.431 ms
> Q5 13611.484 ms -> 13831.946 ms
> Q6 9264.942 ms -> 8734.899 ms
> Q7 9759.026 ms -> 10007.307 ms
> Q8 2473.899 ms -> 2459.225 ms
> Q10 13814.950 ms -> 12255.618 ms
> Q17 49552.298 ms -> 46633.632 ms
>
>
This is strange, I will re-run the test again and post the results soon.



> I haven't really had time to dig into these results yet, so I'm not
> sure how "real" these numbers are and how much is run-to-run jitter,
> EXPLAIN ANALYZE distortion, or whatever.  I think this overall concept
> is good, because there should be cases where it's substantially
> cheaper to preserve the order while gathering tuples from workers than
> to re-sort afterwards.  But this particular set of results is a bit
> lackluster.
>
> --
> Robert Haas
> EnterpriseDB: http://www.enterprisedb.com
> The Enterprise PostgreSQL Company
>



-- 
Rushabh Lathia

Re: [HACKERS] Cache Hash Index meta page.

[Mithun Cy]

On Wed, Jan 11, 2017 at 12:46 AM, Robert Haas  wrote:
> Can we adapt the ad-hoc caching logic in hashbulkdelete() to work with
> this new logic?  Or at least update the comments?
I have introduced a new function _hash_getcachedmetap in patch 11 [1] with
this hashbulkdelete() can use metapage cache instead of saving it locally.

[1] cache_hash_index_meta_page_11.patch

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com

Re: [HACKERS] Cache Hash Index meta page.

[Mithun Cy]

On Fri, Jan 6, 2017 at 11:43 AM, Amit Kapila  wrote:
> Few more comments:
> 1.
> no need to two extra lines, one is sufficient and matches the nearby
> coding pattern.

-- Fixed.

> 2.
> Do you see anywhere else in the code the pattern of using @ symbol in
> comments before function name?

-- Fixed.

> 3.
>
> after this change, i think you can directly use bucket in
> _hash_finish_split instead of pageopaque->hasho_bucket

-- Fixed.

> 4.
>
> Won't the check similar to the existing check (if (*bufp ==
> so->hashso_bucket_buf || *bufp == so->hashso_split_bucket_buf)) just
> above this code will suffice the need?  If so, then you can check it
> once and use it in both places.
>

-- Fixed.

> 5. The reader and insertion algorithm needs to be updated in README.

-- Added info in README.

-- 
Thanks and Regards
Mithun C Y
EnterpriseDB: http://www.enterprisedb.com


cache_hash_index_meta_page_11.patch
Description: Binary data

-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 12, 2017 at 9:14 PM, Gilles Darold  wrote:
> Le 11/01/2017 à 08:39, Michael Paquier a écrit :
>> On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
>>> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
>>> wrote:
 Applied in last version of the patch v18 as well as removing of an
 unused variable.
>>> OK, this looks a lot closer to being committable.  But:
>>>
>>> [long review]
>>
>> Gilles, could you update the patch based on this review from Robert? I
>> am marking the patch as "waiting on author" for the time being. I
>> looked a bit at the patch but did not notice anything wrong with it,
>> but let's see with a fresh version...
> Hi,
>
> My bad, I was thinking that Karl has planned an update of the patch in
> his last post, sorry for my misunderstanding.
>
> Here is attached v19 of the patch that might fix all comment from
> Robert's last review.

OK. I have been looking at this patch.

git diff --check is very unhappy, particularly because of
update_metainfo_datafile().

+
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
File system is used as a name here. In short, "file-system" -> "file
system". I think that it would be a good idea to show up the output of
this file. This could be reworded a bit:
"When logs are written to the file system, the  file includes the location,
the name and the type of the logs currently in use. This provides a
convenient way to find the logs currently in used by the instance."

@@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY
AS t(ls,n);
   

   
+   
pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   
pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
You could just use one line, using squared brackets for the additional
argument. The description looks imprecise, perhaps something like that
would be better: "Log file name currently in use by the logging
collector".

+/* in backend/utils/adt/misc.c and backend/postmaster/syslogger.c */
+/*
+ * Name of file holding the paths, names, and types of the files where current
+ * log messages are written, when the log collector is enabled.  Useful
+ * outside of Postgres when finding the name of the current log file in the
+ * case of time-based log rotation.
+ */
Not mentioning the file names here would be better. If this spreads in
the future updates would likely be forgotten. This paragraph could be
reworked: "configuration file saving meta-data information about the
log files currently in use by the system logging process".

--- a/src/include/miscadmin.h
+++ b/src/include/miscadmin.h
@@ -466,5 +466,4 @@ extern bool has_rolreplication(Oid roleid);
 /* in access/transam/xlog.c */
 extern bool BackupInProgress(void);
 extern void CancelBackup(void);
-
 #endif   /* MISCADMIN_H */
Unrelated diff.

+   /*
+* Force rewriting last log filename when reloading configuration,
+* even if rotation_requested is false, log_destination may have
+* been changed and we don't want to wait the next file rotation.
+*/
+   update_metainfo_datafile();
+
}
I know I am famous for being noisy on such things, but please be
careful with newline use..

+   else
+   {
+   /* Unknown format, no space. Return NULL to caller. */
+   lbuffer[0] = '\0';
+   break;
+   }
Hm. I would think that an ERROR is more useful to the user here.

Perhaps pg_current_logfile() should be marked as STRICT?

Would it make sense to have the 0-argument version be a SRF returning
rows of '(log_destination,location)'? We could just live with this
function I think, and let the caller filter by logging method.

+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
s/logfiles/log files/

Surely the temporary file of current_logfiles should not be included
in base backups (look at basebackup.c). Documentation needs to reflect
that as well. Also, should we have current_logfiles in a base backup?
I would think no.

pg_current_logfile() can be run by *any* user. We had better revoke
its access in system_views.sql!

I have been playing with this patch a bit, and could not make the
system crash :(
-- 
Michael


-- 
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] Patch to implement pg_current_logfile() function

[Michael Paquier]

On Thu, Jan 12, 2017 at 9:55 PM, Karl O. Pinc  wrote:
> On Thu, 12 Jan 2017 13:14:28 +0100
> Gilles Darold  wrote:
>
>> My bad, I was thinking that Karl has planned an update of the patch in
>> his last post, sorry for my misunderstanding.
>
> I was, but have been swept along by events and not gotten to it.

No problem. Thanks for the update.
-- 
Michael


-- 
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] pg_upgrade vs. pg_ctl stop -m fast

[Bruce Momjian]

On Thu, Jan 12, 2017 at 10:17:52AM -0500, Tom Lane wrote:
> Peter Eisentraut  writes:
> > In 9.5, the default pg_ctl stop mode was changed from "smart" to "fast".
> > In pg_upgrade, there is this code:
> > ...
> > I think the last line should be changed to something like
> >   fast ? "-m fast" : "-m smart");
> 
> Ugh.  Clear oversight.
> 
> There is maybe room for a separate discussion about whether pg_upgrade
> *should* be using fast mode, but if so we could remove the "bool fast"
> argument from this function altogether.

Agreed, it should be remove.  Should I do it?

-- 
  Bruce Momjian  http://momjian.us
  EnterpriseDB http://enterprisedb.com

+ As you are, so once was I.  As I am, so you will be. +
+  Ancient Roman grave inscription +


-- 
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] Packages: Again

[Joshua D. Drake]

On 01/12/2017 03:35 PM, Craig Ringer wrote:






So far that's all "that'd be nice, but isn't a technical barrier" stuff.

Package variables for example. When _do_ you _need_ them? For what? (I'm
aware of some uses but "when you need them" helps us not at all).



Well my answer would be, "because we want Oracle people to have an easy 
time migrating". I know that isn't the -hackers answer but clearly there 
is a demand for them. I would note that EDB Advanced server supports 
them, exactly because there is a demand for them.


Again, I am not making the technical argument here. I don't have the 
time to research it. I am making a usability argument for a potentially 
huge portion of database users to allow PostgreSQL to be more attractive 
to them, argument.


I also received this today:

"""
Well, packages make programming much easier. Not only do you keep 
related procedures together, you can also have private package variables 
and the package initialization. Also, packages are units of security, so 
you can grant permissions on the package to the entire group of users 
and if you later modify the package and add a function, you don't need 
to grant it separately.

"""

Sincerely,

JD



--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
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] Proposal for changes to recovery.conf API

[Fujii Masao]

On Thu, Jan 12, 2017 at 6:46 PM, Simon Riggs  wrote:
> On 12 January 2017 at 05:49, Fujii Masao  wrote:
>> On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs  wrote:
>>> On 11 January 2017 at 09:51, Fujii Masao  wrote:
>>>
> 5. recovery.conf parameters are all moved to postgresql.conf, with these 
> changes

 In current design of the patch, when recovery parameters are misconfigured
 (e.g., set recovery_target_timeline to invalid timeline id) and
 the configuration file is reloaded, the startup process emits FATAL error 
 and
 the server goes down. I don't think this is fine. Basically even
 misconfiguration of the parameters should not cause the server crash.
 If invalid settings are supplied, I think that we just should warn them
 and ignore those new settings, like current other GUC is. Thought?
>>>
>>> Thanks for your comments.
>>>
>>> The specification of the recovery target parameters should be different, 
>>> IMHO.
>>>
>>> If the user is performing a recovery and the target of the recovery is
>>> incorrectly specified then it is clear that the recovery cannot
>>> continue with an imprecisely specified target.
>>
>> Could you tell me what case of "the target of the recovery is incorrectly
>> specified" are you thinking of? For example, you're thinking the case where
>> invalid format of timestamp value is specified in recovery_target_time?
>> In this case, I think that we should issue a WARNING and ignore that invalid
>> setting when the configuration file is reloaded. Of course, if it's the 
>> server
>> startup time, such invalid setting should prevent the server from starting 
>> up.
>>
>> Regarding recovery_target_timeline, isn't it better to mark that parameter
>> as PGC_POSTMASTER? I'm not sure if we really want to change the recovery
>> target timeline without server restart and also not sure if that operation is
>> safe.
>
> It's one of the main objectives of the patch to allow user to reset
> parameters online so I don't think we should set PGC_POSTMASTER.

I'm OK to mark the recovery target parameters *except* recovery_target_timeline
as PGC_SIGHUP. But, as I told upthread, I'm wondering whether there is really
the use case where the target timeline needs to be changed online. Also I'm
wondering whether that online change of target timeline is actually safe.

Regards,

-- 
Fujii Masao


-- 
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] plpgsql - additional extra checks

[Jim Nasby]

On 1/11/17 5:54 AM, Pavel Stehule wrote:

+too_many_rows
+
+ 
+  When result is assigned to a variable by INTO clause,
+  checks if query returns more than one row. In this case the assignment
+  is not deterministic usually - and it can be signal some issues in 
design.


Shouldn't this also apply to

var := blah FROM some_table WHERE ...;

?

AIUI that's one of the beefs the plpgsql2 project has.

FWIW, I'd also be in favor of a NOMULTI option to INTO, but I don't see 
any way to do something like that with var := blah FROM.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] WIP: [[Parallel] Shared] Hash

[Peter Geoghegan]

On Wed, Jan 11, 2017 at 7:37 PM, Thomas Munro
 wrote:
> Hmm.  Yes, that is an entirely bogus use of isInterXact.  I am
> thinking about how to fix that with refcounts.

Cool. As I said, the way I'd introduce refcounts would not be very
different from what I've already done -- there'd still be a strong
adherence to the use of resource managers to clean-up, with that
including exactly one particular backend doing the extra step of
deletion. The refcount only changes which backend does that extra step
in corner cases, which is conceptually a very minor change.

>> I don't think it's right for buffile.c to know anything about file
>> paths directly -- I'd say that that's a modularity violation.
>> PathNameOpenFile() is called by very few callers at the moment, all of
>> them very low level (e.g. md.c), but you're using it within buffile.c
>> to open a path to the file that you obtain from shared memory
>
> Hmm.  I'm not seeing the modularity violation.  buffile.c uses
> interfaces already exposed by fd.c to do this:  OpenTemporaryFile,
> then FilePathName to find the path, then PathNameOpenFile to open from
> another process.  I see that your approach instead has client code
> provide more meta data so that things can be discovered, which may
> well be a much better idea.

Indeed, my point was that the metadata thing would IMV be better.
buffile.c shouldn't need to know about file paths, etc. Instead,
caller should pass BufFileImport()/BufFileUnify() simple private state
sufficient for routine to discover all details itself, based on a
deterministic scheme. In my tuplesort patch, that piece of state is:

 /*
+ * BufFileOp is an identifier for a particular parallel operation involving
+ * temporary files.  Parallel temp file operations must be discoverable across
+ * processes based on these details.
+ *
+ * These fields should be set by BufFileGetIdent() within leader process.
+ * Identifier BufFileOp makes temp files from workers discoverable within
+ * leader.
+ */
+typedef struct BufFileOp
+{
+   /*
+* leaderPid is leader process PID.
+*
+* tempFileIdent is an identifier for a particular temp file (or parallel
+* temp file op) for the leader.  Needed to distinguish multiple parallel
+* temp file operations within a given leader process.
+*/
+   int leaderPid;
+   longtempFileIdent;
+} BufFileOp;
+

> Right, that is a problem.  A refcount mode could fix that; virtual
> file descriptors would be closed in every backend using the current
> resource owner, and the files would be deleted when the last one turns
> out the lights.

Yeah. That's basically what the BufFile unification process can
provide you with (or will, once I get around to implementing the
refcount thing, which shouldn't be too hard). As already noted, I'll
also want to make it defer creation of a leader-owned segment, unless
and until that proves necessary, which it never will for hash join.

Perhaps I should make superficial changes to unification in my patch
to suit your work, like rename the field BufFileOp.leaderPid to
BufFileOp.ownerPid, without actually changing any behaviors, except as
noted in the last paragraph. Since you only require that backends be
able to open up some other backend's temp file themselves for a short
while, that gives you everything you need. You'll be doing unification
in backends, and not just within the leader as in the tuplesort patch,
I believe, but that's just fine. All that matters is that you present
all data at once to a consuming backend via unification (since you
treat temp file contents as immutable, this will be true for hash
join, just as it is for tuplesort).

There is a good argument against my making such a tweak, however,
which is that maybe it's clearer to DBAs what's going on if temp file
names have the leader PID in them for all operations. So, maybe
BufFileOp.leaderPid isn't renamed to BufFileOp.ownerPid by me;
instead, you always make it the leader pid, while at the same time
having the leader dole out BufFileOp.tempFileIdent identifiers to each
worker as needed (see how I generate BufFileOps for an idea of what I
mean if it's not immediately clear). That's also an easy change, or at
least will be once the refcount thing is added.

-- 
Peter Geoghegan


-- 
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] many copies of atooid() and oid_cmp()

[Michael Paquier]

On Thu, Jan 12, 2017 at 11:36 PM, Tom Lane  wrote:
> Peter Eisentraut  writes:
>> On 1/11/17 11:25 PM, Tom Lane wrote:
>>> +1 for the concept, but I'm a bit worried about putting atooid() in
>>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>>> applications, for instance.  A more conservative answer would be to
>>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>>> so I do see the consistency of adding this there.  Hard choice.
>
>> How about two copies: one in postgres_fe.h and one in postgres.h?
>
> That seems uglier than either of the other choices.
>
> I don't personally have a huge problem with adding atooid in
> postgres_ext.h, but I thought I'd better flag the potential issue
> to see if anyone else thinks it's a big problem.

FWIW, postgres_ext.h would make the most sense to me. Now, there is
one way to not impose that to frontends linked to libpq which would be
to locate it in some new header in src/include/common/, where both
backend and frontend could reference to it.
-- 
Michael


-- 
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] Packages: Again

[Craig Ringer]

On 13 Jan. 2017 00:54, "Joshua D. Drake"  wrote:

On 01/11/2017 04:12 PM, Craig Ringer wrote:

What aspects / features of packages were the key issues?
>

Unfortunately we didn't get too far into it because the webinar was about
Postgres specifically. That said, I have been doing some followup. Here is
some of it:

because packages[1]

o break the dependency chain (no cascading invalidations when you install a
new package body -- if you have procedures that call procedures --
compiling one will invalidate your database)

o support encapsulation -- I will be allowed to write MODULAR, easy to
understand code -- rather then MONOLITHIC, non-understandable procedures

o increase my namespace measurably. package names have to be unique in a
schema, but I can have many procedures across packages with the same name
without colliding

o support overloading

o support session variables when you need them

o promote overall good coding techniques, stuff that lets you write code
that is modular, understandable, logically grouped together


So far that's all "that'd be nice, but isn't a technical barrier" stuff.

Package variables for example. When _do_ you _need_ them? For what? (I'm
aware of some uses but "when you need them" helps us not at all).

Re: [HACKERS] Retiring from the Core Team

[Merlin Moncure]

On Wed, Jan 11, 2017 at 6:29 PM, Josh Berkus  wrote:
> Hackers:
>
> You will have noticed that I haven't been very active for the past year.
>  My new work on Linux containers and Kubernetes has been even more
> absorbing than I anticipated, and I just haven't had a lot of time for
> PostgreSQL work.
>
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.

Thanks for all your hard work.  FWIW, your blog posts, 'Primary
Keyvil' are some of my favorite of all time!

merlin


-- 
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] GSoC 2017

[Pavel Stehule]

2017-01-12 21:21 GMT+01:00 Jim Nasby :

> On 1/10/17 1:53 AM, Alexander Korotkov wrote:
>
>> 1. What project ideas we have?
>>
>
> Perhaps allowing SQL-only extensions without requiring filesystem files
> would be a good project.
>

Implementation safe evaluation untrusted PL functions - evaluation under
different user under different process.

Regards

Pavel



> --
> Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
> Experts in Analytics, Data Architecture and PostgreSQL
> Data in Trouble? Get it in Treble! http://BlueTreble.com
> 855-TREBLE2 (855-873-2532)
>
>
> --
> 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] Off-by-one oddity in minval for decreasing sequences

[Andrew Dunstan]

On 01/12/2017 03:12 PM, Jim Nasby wrote:

On 1/10/17 8:07 AM, Robert Haas wrote:

This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?


I suspect the number of users that use negative sequence values is so 
small that this is unlikely to be noticed. I can't think of any risk 
to "closing the hole" that you can end up with now. I agree it makes 
sense to sen the minimum value correctly.


Not sure if this necessitates changes in pg_upgrade...



FTR I used them extensively in $previous_job to get out of a nasty problem.

cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



--
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] GSoC 2017

[Jim Nasby]

On 1/10/17 1:53 AM, Alexander Korotkov wrote:

1. What project ideas we have?


Perhaps allowing SQL-only extensions without requiring filesystem files 
would be a good project.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Off-by-one oddity in minval for decreasing sequences

[Jim Nasby]

On 1/10/17 8:07 AM, Robert Haas wrote:

This seems like a sensible argument to me, but maybe somebody's got a
contrary viewpoint?


I suspect the number of users that use negative sequence values is so 
small that this is unlikely to be noticed. I can't think of any risk to 
"closing the hole" that you can end up with now. I agree it makes sense 
to sen the minimum value correctly.


Not sure if this necessitates changes in pg_upgrade...
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] remove floats from bootstrap scanner/parser

[Jim Nasby]

On 1/9/17 9:11 PM, Alvaro Herrera wrote:

I happened to notice that we have dead code to parse floating point
numbers in the boostrap scanner/parser.  This seems unused since forever
-- or, more precisely, I couldn't find any floats in 8.2's postgres.bki
(the oldest I have around).  I would just rip it out, per the attached.


Verified this works for make check. Looks sane to me.
--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] Write Ahead Logging for Hash Indexes

[Jesper Pedersen]

On 12/27/2016 01:58 AM, Amit Kapila wrote:

After recent commit's 7819ba1e and 25216c98, this patch requires a
rebase.  Attached is the rebased patch.



This needs a rebase after commit e898437.

Best regards,
 Jesper



--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

Andres,

* Andres Freund (and...@anarazel.de) wrote:
> On January 12, 2017 10:50:18 AM PST, Stephen Frost  wrote:
> >* Andres Freund (and...@anarazel.de) wrote:
> >> On 2017-01-12 13:40:50 -0500, Stephen Frost wrote:
> >> > * Jim Nasby (jim.na...@bluetreble.com) wrote:
> >> > > The way I see it, either one person can spend an hour or whatever
> >> > > creating an extension once, or every postgres install that's
> >using
> >> > > any of these functions now has yet another hurdle to upgrading.
> >> > 
> >> > I just don't buy this argument, at all.  These functions names are
> >> > certainly not the only things we're changing with PG10 and serious
> >> > monitoring/backup/administration tools are almost certainly going
> >to
> >> > have quite a bit to adjust to with the new release, and that isn't
> >news
> >> > to anyone who works with PG.
> >> 
> >> By that argument we can just do arbitrary backward incompat changes. 
> >We
> >> should aspire to be better than we've been in the past, not use that
> >> past as an excuse for not even trying.
> >
> >When they're changes that are primairly going to affect
> >monitoring/backup/administration tools, yes, I do think we can make
> >just
> >about arbitrary backward-incompatible changes.
> >
> >As Robert mentioned, and I agree with, changing things which will
> >impact
> >regular application usage of PG is a different story and one we should
> >be more cautious about.
> 
> I find it very hard to understand the justification for that, besides that 
> it's strengthening external projects providing monitoring, backup, etc in a 
> compatible way.

That might be understandable if these were entirely arbitrary changes
(which isn't actually what I'm talking about above), but they aren't.
These changes aren't being made just for the sake of making them and are
not entirely arbitrary, as I've already pointed out up-thread.

The point I was making above is that the only reason to not make such
changes is if they really are entirely arbitrary, but I don't think
we'd even be having this discussion if that was the case or that we'd
be split about the question.  We already moved forward with the real
change here- pg_xlog -> pg_wal, it really astounds me that we're having
to argue about what is primairly just clean-up from that change, at
least in my view.

Also, frankly, I don't agree with the notion that this is seriously
going to "strengthen" external projects any more than the regular set of
changes that we're making as we move forward.  Changing these functions
certainly isn't likely to change pgbackrest or barman or check_postgres
uptake any more than the pg_xlog -> pg_wal rename is going to.  Maybe if
we *reverted* that change and then didn't make any other changes then
various hand-written tools would be able to work with PG10 without being
changed, but we might as well call it PG 9.6.2 then.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] pageinspect: Hash index support

[Jesper Pedersen]

Hi,

On 01/11/2017 03:16 PM, Ashutosh Sharma wrote:


I have rephrased it to make it more clear.



Rebased, and removed the compile warn in hashfuncs.c

Best regards,
 Jesper

>From 8a07230b89b97280f0f1d645145da1fd140969c6 Mon Sep 17 00:00:00 2001
From: jesperpedersen 
Date: Thu, 12 Jan 2017 14:00:45 -0500
Subject: [PATCH] Add support for hash index in pageinspect contrib module v13

Authors: Ashutosh Sharma and Jesper Pedersen.
---
 contrib/pageinspect/Makefile  |  10 +-
 contrib/pageinspect/hashfuncs.c   | 548 ++
 contrib/pageinspect/pageinspect--1.5--1.6.sql |  76 
 contrib/pageinspect/pageinspect.control   |   2 +-
 doc/src/sgml/pageinspect.sgml | 148 +++
 src/backend/access/hash/hashovfl.c|  52 +--
 src/include/access/hash.h |   1 +
 7 files changed, 806 insertions(+), 31 deletions(-)
 create mode 100644 contrib/pageinspect/hashfuncs.c
 create mode 100644 contrib/pageinspect/pageinspect--1.5--1.6.sql

diff --git a/contrib/pageinspect/Makefile b/contrib/pageinspect/Makefile
index 87a28e9..d7f3645 100644
--- a/contrib/pageinspect/Makefile
+++ b/contrib/pageinspect/Makefile
@@ -2,13 +2,13 @@
 
 MODULE_big	= pageinspect
 OBJS		= rawpage.o heapfuncs.o btreefuncs.o fsmfuncs.o \
-		  brinfuncs.o ginfuncs.o $(WIN32RES)
+		  brinfuncs.o ginfuncs.o hashfuncs.o $(WIN32RES)
 
 EXTENSION = pageinspect
-DATA = pageinspect--1.5.sql pageinspect--1.4--1.5.sql \
-	pageinspect--1.3--1.4.sql pageinspect--1.2--1.3.sql \
-	pageinspect--1.1--1.2.sql pageinspect--1.0--1.1.sql \
-	pageinspect--unpackaged--1.0.sql
+DATA = pageinspect--1.5.sql pageinspect--1.5--1.6.sql \
+	pageinspect--1.4--1.5.sql pageinspect--1.3--1.4.sql \
+	pageinspect--1.2--1.3.sql pageinspect--1.1--1.2.sql \
+	pageinspect--1.0--1.1.sql pageinspect--unpackaged--1.0.sql
 PGFILEDESC = "pageinspect - functions to inspect contents of database pages"
 
 REGRESS = page btree brin gin
diff --git a/contrib/pageinspect/hashfuncs.c b/contrib/pageinspect/hashfuncs.c
new file mode 100644
index 000..25e9641
--- /dev/null
+++ b/contrib/pageinspect/hashfuncs.c
@@ -0,0 +1,548 @@
+/*
+ * hashfuncs.c
+ *		Functions to investigate the content of HASH indexes
+ *
+ * Copyright (c) 2017, PostgreSQL Global Development Group
+ *
+ * IDENTIFICATION
+ *		contrib/pageinspect/hashfuncs.c
+ */
+
+#include "postgres.h"
+
+#include "access/hash.h"
+#include "access/htup_details.h"
+#include "catalog/pg_am.h"
+#include "funcapi.h"
+#include "miscadmin.h"
+#include "utils/builtins.h"
+
+PG_FUNCTION_INFO_V1(hash_page_type);
+PG_FUNCTION_INFO_V1(hash_page_stats);
+PG_FUNCTION_INFO_V1(hash_page_items);
+PG_FUNCTION_INFO_V1(hash_bitmap_info);
+PG_FUNCTION_INFO_V1(hash_metapage_info);
+
+#define IS_HASH(r) ((r)->rd_rel->relam == HASH_AM_OID)
+
+/* 
+ * structure for single hash page statistics
+ * 
+ */
+typedef struct HashPageStat
+{
+	uint32		live_items;
+	uint32		dead_items;
+	uint32		page_size;
+	uint32		free_size;
+
+	/* opaque data */
+	BlockNumber hasho_prevblkno;
+	BlockNumber hasho_nextblkno;
+	Bucket		hasho_bucket;
+	uint16		hasho_flag;
+	uint16		hasho_page_id;
+} HashPageStat;
+
+
+/*
+ * Verify that the given bytea contains a HASH page, or die in the attempt.
+ * A pointer to the page is returned.
+ */
+static Page
+verify_hash_page(bytea *raw_page, int flags)
+{
+	Page		page;
+	int			raw_page_size;
+	HashPageOpaque pageopaque;
+
+	raw_page_size = VARSIZE(raw_page) - VARHDRSZ;
+
+	if (raw_page_size != BLCKSZ)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("input page too small"),
+ errdetail("Expected size %d, got %d",
+		   BLCKSZ, raw_page_size)));
+
+	page = VARDATA(raw_page);
+
+	if (PageIsNew(page))
+		ereport(ERROR,
+(errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index table contains unexpected zero page")));
+
+	if (PageGetSpecialSize(page) != MAXALIGN(sizeof(HashPageOpaqueData)))
+		ereport(ERROR,
+(errcode(ERRCODE_INDEX_CORRUPTED),
+ errmsg("index table contains corrupted page")));
+
+	pageopaque = (HashPageOpaque) PageGetSpecialPointer(page);
+	if (pageopaque->hasho_page_id != HASHO_PAGE_ID)
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+ errmsg("page is not a HASH page"),
+ errdetail("Expected %08x, got %08x.",
+		   HASHO_PAGE_ID, pageopaque->hasho_page_id)));
+
+	if (!(pageopaque->hasho_flag & flags))
+		ereport(ERROR,
+(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
+		 errmsg("page is not a hash page of expected type"),
+			errdetail("Expected hash page type: %08x got: %08x.",
+flags, pageopaque->hasho_flag)));
+	return page;
+}
+
+/* -
+ * GetHashPageStatistics()
+ *
+ * Collect statistics of single hash page
+ * -
+ */
+static void

Re: [HACKERS] WARM and indirect indexes

[Jim Nasby]

On 1/11/17 8:09 PM, Pavan Deolasee wrote:

The other thing the patch changes is how update-chain is maintained. In
order to quickly find the root offset while updating a tuple, we now
store the root offset in the t_ctid field of the last tuple in the chain
and use a separate bit to mark end-of-the-chain (instead of relying of
t_ctid = t_self check). That can lead to problems if chains are not
maintained or followed correctly. These changes are in the first patch
of the patch series and if you've any suggestions on how to improve that
or solidify chain following, please let me know. I was looking for some
way to hide t_ctid field to ensure that the links are only accessed via
some standard API.


AIUI, that's going to affect every method of heap access except for 
index scans that can skip the heap due to being all-visible. That means 
the risk here is comparable to the MXID changes.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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: pg_stat_statements query normalization issues with combined queries

[Fabien COELHO]

About having a pointer to the initial string from RawStmt, Query & 
PlannedStmt:


I remembered one reason why we haven't done this: it's unclear how we'd 
handle copying if we do it. If, say, Query contains a "char *" pointer 
then you'd expect copyObject() to pstrdup that string, [..., So] We'd 
need to work out a way of managing multiple Queries carrying references 
to the same source string, and it's not clear how to do that reasonably.


For me it would be shared, but then it may break some memory management 
hypothesis downstream.


So for now, it remains the path of least resistance for Portals and 
CachedPlans to contain a list of Query or PlannedStmt objects and a 
separate source string.


Ok. Thanks for the explanation.

--
Fabien.


--
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: pg_stat_statements query normalization issues with combined queries

[Fabien COELHO]

[...] it seems a lot cleaner to me.


I agree.


[...] It's safe in backend-legal encodings.


Good:-)

[...] The trouble for statements like EXPLAIN is that we replace the 
sub-statement during parse analysis, [...]


Ok. So Node* is fine to keep it generic.

--
Fabien.


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

* Tom Lane (t...@sss.pgh.pa.us) wrote:
> Stephen Frost  writes:
> > I just don't buy this argument, at all.  These functions names are
> > certainly not the only things we're changing with PG10 and serious
> > monitoring/backup/administration tools are almost certainly going to
> > have quite a bit to adjust to with the new release, and that isn't news
> > to anyone who works with PG.
> 
> Hmm --- we've been conducting this argument in a vacuum, but you're right,
> we should consider what else is changing in v10.  If you can point to
> already-committed changes that mean that code using these functions will
> almost certainly need changes anyway for v10, then that would greatly
> weaken the argument for providing aliases.

We changed the pg_xlog directory to be pg_wal, that's certainly going to
have an impact on monitoring and backup tools.

We've also made changes in, at least, what's reported in
pg_stat_activity and there's been discussions about changing it further
(list background workers or not, etc), and in pg_stat_replication (with
the addition of quorum-based sync rep).  In fact, these kinds of changes
are almost certainly going to require more work for tool authors to deal
with than just a simple function name change.

And that's only with a few minutes of looking at the commit log and I
don't doubt that there's more and that we're going to have even more
before feature freeze that a serious monitoring tool will have to be
updated for.

As long as we properly include these changes in the release notes for
tool authors, I really don't see any of them as a big deal.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On January 12, 2017 10:50:18 AM PST, Stephen Frost  wrote:
>* Andres Freund (and...@anarazel.de) wrote:
>> On 2017-01-12 13:40:50 -0500, Stephen Frost wrote:
>> > * Jim Nasby (jim.na...@bluetreble.com) wrote:
>> > > The way I see it, either one person can spend an hour or whatever
>> > > creating an extension once, or every postgres install that's
>using
>> > > any of these functions now has yet another hurdle to upgrading.
>> > 
>> > I just don't buy this argument, at all.  These functions names are
>> > certainly not the only things we're changing with PG10 and serious
>> > monitoring/backup/administration tools are almost certainly going
>to
>> > have quite a bit to adjust to with the new release, and that isn't
>news
>> > to anyone who works with PG.
>> 
>> By that argument we can just do arbitrary backward incompat changes. 
>We
>> should aspire to be better than we've been in the past, not use that
>> past as an excuse for not even trying.
>
>When they're changes that are primairly going to affect
>monitoring/backup/administration tools, yes, I do think we can make
>just
>about arbitrary backward-incompatible changes.
>
>As Robert mentioned, and I agree with, changing things which will
>impact
>regular application usage of PG is a different story and one we should
>be more cautious about.

I find it very hard to understand the justification for that, besides that it's 
strengthening external projects providing monitoring, backup, etc in a 
compatible way.

Andres

-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

* Andres Freund (and...@anarazel.de) wrote:
> On 2017-01-12 13:40:50 -0500, Stephen Frost wrote:
> > * Jim Nasby (jim.na...@bluetreble.com) wrote:
> > > The way I see it, either one person can spend an hour or whatever
> > > creating an extension once, or every postgres install that's using
> > > any of these functions now has yet another hurdle to upgrading.
> > 
> > I just don't buy this argument, at all.  These functions names are
> > certainly not the only things we're changing with PG10 and serious
> > monitoring/backup/administration tools are almost certainly going to
> > have quite a bit to adjust to with the new release, and that isn't news
> > to anyone who works with PG.
> 
> By that argument we can just do arbitrary backward incompat changes.  We
> should aspire to be better than we've been in the past, not use that
> past as an excuse for not even trying.

When they're changes that are primairly going to affect
monitoring/backup/administration tools, yes, I do think we can make just
about arbitrary backward-incompatible changes.

As Robert mentioned, and I agree with, changing things which will impact
regular application usage of PG is a different story and one we should
be more cautious about.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Stephen Frost  writes:
> I just don't buy this argument, at all.  These functions names are
> certainly not the only things we're changing with PG10 and serious
> monitoring/backup/administration tools are almost certainly going to
> have quite a bit to adjust to with the new release, and that isn't news
> to anyone who works with PG.

Hmm --- we've been conducting this argument in a vacuum, but you're right,
we should consider what else is changing in v10.  If you can point to
already-committed changes that mean that code using these functions will
almost certainly need changes anyway for v10, then that would greatly
weaken the argument for providing aliases.

regards, tom lane


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Andres Freund]

On 2017-01-12 13:40:50 -0500, Stephen Frost wrote:
> Jim,
> 
> * Jim Nasby (jim.na...@bluetreble.com) wrote:
> > The way I see it, either one person can spend an hour or whatever
> > creating an extension once, or every postgres install that's using
> > any of these functions now has yet another hurdle to upgrading.
> 
> I just don't buy this argument, at all.  These functions names are
> certainly not the only things we're changing with PG10 and serious
> monitoring/backup/administration tools are almost certainly going to
> have quite a bit to adjust to with the new release, and that isn't news
> to anyone who works with PG.

By that argument we can just do arbitrary backward incompat changes.  We
should aspire to be better than we've been in the past, not use that
past as an excuse for not even trying.

Greetings,

Andres Freund


-- 
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] merging some features from plpgsql2 project

[Jim Nasby]

On 1/11/17 12:07 PM, Pavel Stehule wrote:

PL/SQL = ADA + Oracle SQL; -- but sometimes the result is not perfect -
Ada was not designed be integrated with SQL

...

There is a language that is much better integrated with SQL - SQL/PSM


I think it is worth considering ways to increase compatibility with 
plsql, as well as pulling PSM into core. The former would be to help 
migrating from Oracle; the latter would be to provide everyone a cleaner 
built-in PL. (IMHO a PLSQL equivalent could certainly be an external 
extension).

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Stephen Frost]

Jim,

* Jim Nasby (jim.na...@bluetreble.com) wrote:
> The way I see it, either one person can spend an hour or whatever
> creating an extension once, or every postgres install that's using
> any of these functions now has yet another hurdle to upgrading.

I just don't buy this argument, at all.  These functions names are
certainly not the only things we're changing with PG10 and serious
monitoring/backup/administration tools are almost certainly going to
have quite a bit to adjust to with the new release, and that isn't news
to anyone who works with PG.

Thanks!

Stephen


signature.asc
Description: Digital signature

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

[Tom Lane]

I wrote:
> Fabien COELHO  writes:
>> One point bothered me a bit when looking at the initial code, that your 
>> refactoring has not changed: the location is about a string which is not 
>> accessible from the node, so that the string must be kept elsewhere and 
>> passed as a second argument here and there. ISTM that it would make some 
>> sense to have the initial string directly available from RawStmt, Query 
>> and PlannedStmt.

> Yeah, perhaps, but that would be something to tackle as a separate round
> of changes I think.

I remembered one reason why we haven't done this: it's unclear how we'd
handle copying if we do it.  If, say, Query contains a "char *" pointer
then you'd expect copyObject() to pstrdup that string, but we really don't
want that to happen in scenarios where a single source string generated a
large number of Query trees.  That's not an academic concern, we've gotten
real field bug reports when we broke it --- cf commit 1591fcbec.  We'd
need to work out a way of managing multiple Queries carrying references
to the same source string, and it's not clear how to do that reasonably.
So for now, it remains the path of least resistance for Portals and
CachedPlans to contain a list of Query or PlannedStmt objects and a
separate source string.

regards, tom lane


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Kevin Grittner]

On Thu, Jan 12, 2017 at 12:12 PM, Tom Lane  wrote:
> Vladimir Rusinov  writes:
>> On Thu, Jan 12, 2017 at 4:57 PM, Euler Taveira  wrote:
>>> As Robert suggested in the other email: extension to create old names.
>
>> I don't follow the reasoning for the extension. It seem to have downsides
>> of both alternatives combined: we still break people's code, and we add
>> even more maintenance burden than just keeping aliases.
>
> Yeah, I'm not terribly for the extension idea.  Robert cited the precedent
> of contrib/tsearch2, but I think the history of that is a good argument
> against this: tsearch2 is still there 9 years later and there's no
> indication that we'll ever get rid of it.  We can let things rot
> indefinitely in core too.  If we do ever agree to get rid of the aliases,
> stripping them out of pg_proc.h will not be any harder than removing
> a contrib directory would be.

How about just leaving it to someone who cares about the aliases to
post such an extension at pgxn.org (or anywhere else they like).
It can be around as long as someone cares enough to maintain it.

I guess that makes my vote -1 on aliases by the project.

--
Kevin Grittner
EDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] WARM and indirect indexes

[Robert Haas]

On Wed, Jan 11, 2017 at 11:09 PM, Pavan Deolasee
 wrote:
> I think as a developer of the patch, what I would like to know is what can
> we do address concerns raised by you? What kind of tests you would like to
> do to get confidence in the patch?

Well, off the top of my head, I'd say that there are basically four
things that can be done to improve the quality of patches.  These
things are, of course, not a secret:

1. Detailed manual testing.
2. Stress testing.
3. Regression testing (pg_regress, isolation, TAP) perhaps driven by
code coverage reports.
4. Detailed code review by highly-qualified individuals.

For a patch of this type, I highly recommend stress testing.  Amit and
his colleagues have done a huge amount of stress testing of the hash
index patches and that's turned up quite a few bugs; sometimes the
tests had to run for many hours before any failure was provoked.

But my real point here is not that reviewing or testing the WARM patch
is any different or more difficult than for any other patch.  Rather,
the issue is that the stakes are higher.  Whatever a committer would
do (or ask to have done) for a patch of ordinary consequence should be
done ten times over for that one, not because it's more likely to have
bugs but because any bugs that it does have will hurt more.  There's a
time for committing and moving on and a time for extreme paranoia.
IMHO, this is the latter.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Jim Nasby]

On 1/12/17 10:12 AM, Tom Lane wrote:

Yeah, I'm not terribly for the extension idea.  Robert cited the precedent
of contrib/tsearch2, but I think the history of that is a good argument
against this: tsearch2 is still there 9 years later and there's no
indication that we'll ever get rid of it.  We can let things rot
indefinitely in core too.  If we do ever agree to get rid of the aliases,
stripping them out of pg_proc.h will not be any harder than removing
a contrib directory would be.


Is there any burden to carrying tsearch2 around? Have we formally marked 
it as deprecated?


The way I see it, either one person can spend an hour or whatever 
creating an extension once, or every postgres install that's using any 
of these functions now has yet another hurdle to upgrading.


If PGXN was better supported by the community maybe the answer would be 
to just throw an extension up there. But that's certainly not the case.

--
Jim Nasby, Data Architect, Blue Treble Consulting, Austin TX
Experts in Analytics, Data Architecture and PostgreSQL
Data in Trouble? Get it in Treble! http://BlueTreble.com
855-TREBLE2 (855-873-2532)


--
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: pg_stat_statements query normalization issues with combined queries

[Tom Lane]

Fabien COELHO  writes:
>> [...] Furthermore, the output of pg_plan_queries is now always a list of 
>> PlannedStmt nodes, even for utility statements.

> I stopped exactly there when I tried: I thought that changing that would 
> be enough to get a reject because it would be considered much too invasive 
> in too many places for fixing a small bug.

Well, I don't know that we'd have bothered to change this without a
reason, but now that the effort's been put in, it seems a lot cleaner
to me.

> I noticed that you also improved the pgss fix and tests. Good. I was 
> unsure about removing spaces at the end of the query because of possible 
> encoding issues.

It's safe in backend-legal encodings.  I agree it could be problematic
in, say, SJIS, but we disallow that as a backend encoding precisely
because of this type of hazard.

> The update end trick is nice to deal with empty statements. I wonder 
> whether the sub "query" in Copy, View, CreateAs, Explain, Prepare, 
> Execute, Declare... could be typed RawStmt* instead of Node*.

I did that for some of them, eg CopyStmt.  The trouble for statements like
EXPLAIN is that we replace the sub-statement during parse analysis, so
that the field points to either a raw grammar output node or to a Query
depending on when you look.  If we wanted to get rid of using a Node*
field there, we'd have to invent two separate struct types to represent
raw EXPLAIN and post-parse-analyze EXPLAIN (analogously to the way that,
say, ColumnRef and Var are distinct node types).  It doesn't seem worth
the trouble to me.

> One point bothered me a bit when looking at the initial code, that your 
> refactoring has not changed: the location is about a string which is not 
> accessible from the node, so that the string must be kept elsewhere and 
> passed as a second argument here and there. ISTM that it would make some 
> sense to have the initial string directly available from RawStmt, Query 
> and PlannedStmt.

Yeah, perhaps, but that would be something to tackle as a separate round
of changes I think.  This might also tie into the nearby thread about
having that string available to pass to parallel workers.  The history
here is that we didn't use to keep the source string around after parsing,
but that's not the case anymore.  But we've not entirely accepted that
in all the internal APIs ...

regards, tom lane


-- 
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] Retiring from the Core Team

[Roberto Mello]

On Wed, Jan 11, 2017 at 8:29 PM, Josh Berkus  wrote:

>
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.


Thank you for all your hard work and contributions to the project over the
years Josh. Sometimes the work of PR and documentation (among others) gets
sidelined and not given the appropriate importance.

I wish you all the best in your current and future endeavors, and hope our
paths will cross again sometime.

Roberto

Re: [HACKERS] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Tom Lane]

Vladimir Rusinov  writes:
> On Thu, Jan 12, 2017 at 4:57 PM, Euler Taveira  wrote:
>> As Robert suggested in the other email: extension to create old names.

> I don't follow the reasoning for the extension. It seem to have downsides
> of both alternatives combined: we still break people's code, and we add
> even more maintenance burden than just keeping aliases.

Yeah, I'm not terribly for the extension idea.  Robert cited the precedent
of contrib/tsearch2, but I think the history of that is a good argument
against this: tsearch2 is still there 9 years later and there's no
indication that we'll ever get rid of it.  We can let things rot
indefinitely in core too.  If we do ever agree to get rid of the aliases,
stripping them out of pg_proc.h will not be any harder than removing
a contrib directory would be.

regards, tom lane


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Vladimir Rusinov]

On Thu, Jan 12, 2017 at 4:57 PM, Euler Taveira  wrote:

> As Robert suggested in the other email: extension to create old names.


I don't follow the reasoning for the extension. It seem to have downsides
of both alternatives combined: we still break people's code, and we add
even more maintenance burden than just keeping aliases.

-- 
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047


smime.p7s
Description: S/MIME Cryptographic Signature

Re: [HACKERS] BUG: pg_stat_statements query normalization issues with combined queries

[Fabien COELHO]

Hello Tom,


Yeah, I doubt that this technique for getting the raw locations in the
grammar+lexer works reliably.


It worked reliably on all tests, and the assumption seemed sound to me, 
given the one-look-ahead property and that statement reductions can only 
triggered when ';' or end of input comes.



Anyway, I decided to see what it would take to do it the way I had
in mind, which was to stick a separate RawStmt node atop each statement
parsetree.  The project turned out a bit larger than I hoped :-(,


Indeed: I tried it as it looked elegant, but it did not show to be the 
simple thing you announced...


[...] Furthermore, the output of pg_plan_queries is now always a list of 
PlannedStmt nodes, even for utility statements.


I stopped exactly there when I tried: I thought that changing that would 
be enough to get a reject because it would be considered much too invasive 
in too many places for fixing a small bug.



So this ends up costing one extra palloc per statement, or two extra
in the case of a utility statement, but really that's quite negligible
compared to everything else that happens in processing a statement.



As against that, I think it makes the related code a lot clearer.


Having spent some time there, I agree that a refactoring and cleanup was 
somehow needed...



The sheer size of the patch is a bit more than Fabien's patch,


Yep, nearly twice as big and significantly more complex, but the result is 
a definite improvement.


but what it is touching is not per-statement-type code but code that 
works generically with lists of statements.  So I think it's less likely 
to cause problems for other patches.


Patch applies (with "patch", "git apply" did not like it though), 
compiles, overall make check ok, pgss make check ok as well. I do not know 
whether the coverage of pg tests is enough to know whether anything is 
broken...


I noticed that you also improved the pgss fix and tests. Good. I was 
unsure about removing spaces at the end of the query because of possible 
encoding issues.


The update end trick is nice to deal with empty statements. I wonder 
whether the sub "query" in Copy, View, CreateAs, Explain, Prepare, 
Execute, Declare... could be typed RawStmt* instead of Node*. That would 
avoid some casts in updateRawStmtEnd, but maybe add some other elsewhere.


One point bothered me a bit when looking at the initial code, that your 
refactoring has not changed: the location is about a string which is not 
accessible from the node, so that the string must be kept elsewhere and 
passed as a second argument here and there. ISTM that it would make some 
sense to have the initial string directly available from RawStmt, Query 
and PlannedStmt.


--
Fabien.


--
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] Improvements in psql hooks for variables

[Tom Lane]

"Daniel Verite"  writes:
> [ psql-var-hooks-v6.patch ]

I took a quick look through this.  It seems to be going in generally
the right direction, but here's a couple of thoughts:

* It seems like you're making life hard for yourself, and confusing for
readers, by having opposite API conventions at different levels.  You've
got ParseVariableBool returning the parsed value as function result with
validity flag going into an output parameter, but the boolean variable
hooks do it the other way.

I'm inclined to think that the best choice is for the function result
to be the ok/not ok flag, and pass the variable-to-be-modified as an
output parameter.  That fits better with the notion that the variable
is not to be modified on failure.  You're having to change every caller
of ParseVariableBool anyway, so changing them a bit more doesn't seem
like a problem.  I think actually you don't need generic_boolean_hook()
at all if you do that; it appears to do nothing except fix this impedance
mismatch.

Also, why aren't you using ParseVariableBool's existing ability to report
errors?  It seems like this:

  else if (value)
! {
! boolvalid;
! boolnewval = ParseVariableBool(value, NULL, );
! if (valid)
! popt->topt.expanded = newval;
! else
! {
! psql_error("unrecognized value \"%s\" for \"%s\"\n",
!value, param);
! return false;
! }
! }

is really the hard way and you could have just done

- popt->topt.expanded = ParseVariableBool(value, param);
+ success = ParseVariableBool(value, param, >topt.expanded);


I'd do it the same way for ParseCheckVariableNum.  Also, is there a reason
why that's adding new code rather than changing ParseVariableNum?
I think if we're going to have a policy that bool variables must be valid
bools, there's no reason not to insist similarly for integers.

* More attention is needed to comments.  Most glaringly, you've changed
the API for VariableAssignHook without any attention to the API spec
above that typedef.  (That comment block is a bit confused anyway, since
half of it is an overall explanation of what the module does and the
other half is an API spec for the hooks.  I think I'd move the overall
explanation into the file header comment.)  Also, I don't like this way
of explaining an output parameter:

+  * "valid" points to a bool reporting whether the value was valid.

because it's not really clear that the function is setting that value
rather than expecting it to be set to something by the caller.
Assuming you take my advice in the previous point, you could document
ParseVariableBool and ParseVariableNum along the lines of

 * Returns true if string contents represent a valid value, false if not.
 * If the string is valid, the value is stored into *value, else *value
 * remains unchanged.


regards, tom lane


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Euler Taveira]

On 12-01-2017 10:56, David Steele wrote:
>> So, to sum up things on this thread, here are the votes about the use
>> of aliases or a pure breakage:
>> - No to aliases, shake the world: Stephen, Tom, David F, Vik, Bruce M => 5
>> - Yes to aliases: Michael P, Andres, Peter E., Cynthia S, Jim N,
>> Vladimir, Simon R, Fujii-san => 8
>> If I misunderstood things, please feel free to speak up.
> 
> I'm also in the "no to aliases" camp.
> 
+1. Even if we don't document aliases, one day some applications will
break when someone decided to clean up this code. Let's do it smoothly.
As Robert suggested in the other email: extension to create old names.


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


-- 
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] Packages: Again

[Joshua D. Drake]

On 01/11/2017 04:12 PM, Craig Ringer wrote:


What aspects / features of packages were the key issues?


Unfortunately we didn't get too far into it because the webinar was 
about Postgres specifically. That said, I have been doing some followup. 
Here is some of it:


because packages[1]

o break the dependency chain (no cascading invalidations when you 
install a new package body -- if you have procedures that call 
procedures -- compiling one will invalidate your database)


o support encapsulation -- I will be allowed to write MODULAR, easy to 
understand code -- rather then MONOLITHIC, non-understandable procedures


o increase my namespace measurably. package names have to be unique in a 
schema, but I can have many procedures across packages with the same 
name without colliding


o support overloading

o support session variables when you need them

o promote overall good coding techniques, stuff that lets you write code 
that is modular, understandable, logically grouped together


Note: I am not arguing the technical merits here. My goal is 100%, how 
do we get Oracle folks a true Open Source Oracle alternative.


As I get more from people, I will post.

Sincerely,

JD

1. 
https://asktom.oracle.com/pls/asktom/f?p=100:11:0P11_QUESTION_ID:7452431376537




--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
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] Retiring from the Core Team

[Euler Taveira]

On 11-01-2017 21:29, Josh Berkus wrote:
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.
> 
Thanks for your hard work! Your contributions were very important and
valuable for community growth. I'm sure you will continue to advocate
PostgreSQL.


-- 
   Euler Taveira   Timbira - http://www.timbira.com.br/
   PostgreSQL: Consultoria, Desenvolvimento, Suporte 24x7 e Treinamento


-- 
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] Retiring from the Core Team

[Joshua D. Drake]

On 01/12/2017 07:48 AM, Jonathan Katz wrote:



On Jan 11, 2017, at 7:29 PM, Josh Berkus  wrote:

It's been a long, fun ride, and I'm proud of the PostgreSQL we have
today: both the database, and the community.  Thank you for sharing it
with me.


Thank you for all of your contributions to the PostgreSQL community and paving 
the way for many of the advocacy efforts that we can do today through your 
leadership.  I wish you the best of luck in all of your endeavors!



Josh,

Thank you for all your service. May your new adventures be as fulfilling!

JD


--
Command Prompt, Inc.  http://the.postgres.company/
+1-503-667-4564
PostgreSQL Centered full stack support, consulting and development.
Everyone appreciates your honesty, until you are honest with them.
Unless otherwise stated, opinions are my own.


--
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Vladimir Rusinov]

On Thu, Jan 12, 2017 at 5:49 AM, Michael Paquier 
wrote:

> The patch still updates a bunch of .po files. Those are normally
> refreshed with the translation updates, so they had better be removed.
> Other than that, the patch looks in good shape to me so switch to
> "Ready for committer".
>

Gotcha. Excluded these from the new version of the patch. It's actually
same as above but produced with some git :(exclude) magic.


> So, to sum up things on this thread, here are the votes about the use
> of aliases or a pure breakage:
> - No to aliases, shake the world: Stephen, Tom, David F, Vik, Bruce M => 5
> - Yes to aliases: Michael P, Andres, Peter E., Cynthia S, Jim N,
> Vladimir, Simon R, Fujii-san => 8
> If I misunderstood things, please feel free to speak up.
>

Counting amendments and more votes down the thread:

- no aliases: Stephen, Tom, David F, Vik, Bruce M, David Steele, Robert
Haas => 7
- yes to aliases: Michael P, Andres, Peter E., Cynthia S, Jim N, Vladimir,
Simon R => 7
- avoid renaming altogether: Fujii-san => 1

Duh. Folks, you are not making it easy. :) Looks like we need one more vote.

-- 
Vladimir Rusinov
Storage SRE, Google Ireland

Google Ireland Ltd.,Gordon House, Barrow Street, Dublin 4, Ireland
Registered in Dublin, Ireland
Registration Number: 368047
diff --git a/contrib/bloom/t/001_wal.pl b/contrib/bloom/t/001_wal.pl
index 56c6618d3d..dc907baf87 100644
--- a/contrib/bloom/t/001_wal.pl
+++ b/contrib/bloom/t/001_wal.pl
@@ -16,7 +16,7 @@ sub test_index_replay
 	# Wait for standby to catch up
 	my $applname = $node_standby->name;
 	my $caughtup_query =
-"SELECT pg_current_xlog_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
+"SELECT pg_current_wal_location() <= write_location FROM pg_stat_replication WHERE application_name = '$applname';";
 	$node_master->poll_query_until('postgres', $caughtup_query)
 	  or die "Timed out while waiting for standby 1 to catch up";
 
diff --git a/contrib/test_decoding/expected/ddl.out b/contrib/test_decoding/expected/ddl.out
index c104c4802d..275c84a450 100644
--- a/contrib/test_decoding/expected/ddl.out
+++ b/contrib/test_decoding/expected/ddl.out
@@ -58,7 +58,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 slot_name|plugin | slot_type | active | catalog_xmin_set | data_xmin_not_set | some_wal 
 -+---+---++--+---+--
diff --git a/contrib/test_decoding/sql/ddl.sql b/contrib/test_decoding/sql/ddl.sql
index 89b8b8f780..49dad39b50 100644
--- a/contrib/test_decoding/sql/ddl.sql
+++ b/contrib/test_decoding/sql/ddl.sql
@@ -29,7 +29,7 @@ SELECT 'init' FROM pg_create_logical_replication_slot('regression_slot', 'test_d
 SELECT slot_name, plugin, slot_type, active,
 NOT catalog_xmin IS NULL AS catalog_xmin_set,
 xmin IS NULl  AS data_xmin_not_set,
-pg_xlog_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
+pg_wal_location_diff(restart_lsn, '0/0100') > 0 AS some_wal
 FROM pg_replication_slots;
 
 /*
diff --git a/doc/src/sgml/backup.sgml b/doc/src/sgml/backup.sgml
index 6eaed1efbe..e37f101277 100644
--- a/doc/src/sgml/backup.sgml
+++ b/doc/src/sgml/backup.sgml
@@ -726,9 +726,9 @@ test ! -f /mnt/server/archivedir/000100A90065  cp pg_wal/0
 

 Also, you can force a segment switch manually with
-pg_switch_xlog if you want to ensure that a
-just-finished transaction is archived as soon as possible.  Other utility
-functions related to WAL management are listed in pg_switch_wal if you want to ensure that a just-finished
+transaction is archived as soon as possible.  Other utility functions
+related to WAL management are listed in .

 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e31868ba..a5bfb4b306 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -17925,13 +17925,13 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_create_restore_point


-pg_current_xlog_flush_location
+pg_current_wal_flush_location


-pg_current_xlog_insert_location
+pg_current_wal_insert_location


-pg_current_xlog_location
+pg_current_wal_location


 pg_start_backup
@@ -17946,24 +17946,25 @@ SELECT set_config('log_statement_stats', 'off', false);
 pg_backup_start_time


-pg_switch_xlog
+pg_switch_wal


-pg_xlogfile_name
+pg_wal_file_name


-pg_xlogfile_name_offset
+pg_wal_file_name_offset


-pg_xlog_location_diff
+pg_wal_location_diff

Re: [HACKERS] Retiring from the Core Team

[Jonathan Katz]

> On Jan 11, 2017, at 7:29 PM, Josh Berkus  wrote:
> 
> It's been a long, fun ride, and I'm proud of the PostgreSQL we have
> today: both the database, and the community.  Thank you for sharing it
> with me.

Thank you for all of your contributions to the PostgreSQL community and paving 
the way for many of the advocacy efforts that we can do today through your 
leadership.  I wish you the best of luck in all of your endeavors!

Sincerely,

Jonathan



-- 
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 bug in 9.6

[Robert Haas]

On Wed, Jan 11, 2017 at 2:45 AM, Etsuro Fujita
 wrote:
> As I said before, that might be fine for 9.6, but I don't think it's a good
> idea to search the pathlist because once we support parameterized foreign
> join paths, which is on my TODOs, we would have to traverse through the
> possibly-lengthy pathlist to find a local-join path, as mentioned in [3].

I'm not sure that's really going to be a problem.  The number of
possible parameterizations that need to be considered isn't usually
very big.  I bet the path list will have ten or a few tens of entries
in it, not a hundred or a thousand.  Traversing it isn't that
expensive.

That having been said, I haven't read the patches, so I'm not really
up to speed on the bigger issues here.  But surely it's more important
to get the overall design right than to worry about the cost of
walking the pathlist or worrying about the cost of an extra function
call (?!).

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] Retiring from the Core Team

[Dave Cramer]

On 11 January 2017 at 16:29, Josh Berkus  wrote:

> Hackers:
>
> You will have noticed that I haven't been very active for the past year.
>  My new work on Linux containers and Kubernetes has been even more
> absorbing than I anticipated, and I just haven't had a lot of time for
> PostgreSQL work.
>
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.
>
I joined the PostgreSQL Core Team in 2003.  I decided to take on project
> advocacy, with the goal of making PostgreSQL one of the top three
> databases in the world.  Thanks to the many contributions by both
> advocacy volunteers and developers -- as well as the efforts by
> companies like EnterpriseDB and Heroku -- we've achieved that goal.
> Along the way, we proved that community ownership of an OSS project can
> compete with, and ultimately outlast, venture-funded startups.
>


Thanks for your hard work. PostgreSQL has come a long way since then!


Dave Cramer

da...@postgresintl.com
www.postgresintl.com

Re: [HACKERS] UNDO and in-place update

[Robert Haas]

On Wed, Jan 11, 2017 at 5:18 AM, Amit Kapila  wrote:
> Moving further, I have thought about consistent reads and different
> formats for storing undo with pros and cons of each format.
>
> Consistent Reads
> 
> If the page is modified by any transaction that is not visible to read
> transaction's snapshot, we have to reconstruct a copy of the page.
> Now there could be multiple ways to achieve it:
>
> 1. Apply the undo for the transactions that are not visible to read
> transaction and keep the copy of the page.
> 2. Apply the undo for all the transactions in TPD entry of a page.
>
> I think we can't do second because this can create rows which are not
> consistent due to apply of undo log for transactions which are visible
> to read transaction.  Now, where the first approach will work, but it
> might turn out to be inefficient if not implemented carefully, as for
> each read transaction we need to create separate copies of the page
> where a different set of transactions are not visible to different
> read transactions. Even for read transactions to which the same set of
> transactions are not visible, we need some way to recognize the
> version of the page that can be used, probably try with all the
> version of pages and see if any of the existing versions can serve the
> purpose.  I think we also need some way to link the different versions
> of the page in shared buffers so that before trying to create a new
> version of the page we can look at linked versions.
>
> Yet another consideration in this area could be to perform consistent
> reads differently for index scans.  For index scan, we will receive a
> particular TID to read, so, in this case, we can try to reconstruct
> the old image of that particular row.  Now, whereas this sounds
> efficient, but we might need to reconstruct the old image of rows
> multiple times for different read transactions. The effect will be
> magnificent when we have to perform this for multiple rows of a page.
> I think it might be better to always construct a complete copy of page
> if one or more transactions that have changed the page are not visible
> to read snapshot.

I was thinking about this whole area somewhat differently.  We don't
really need a complete imagine of the page.  We just need to find the
correct version of each tuple that is visible.  So I would suggest
that we consider avoiding page reconstruction altogether.  A
sequential scan iterates through each TID on the page and says "is
this tuple visible?".  Right now the answer is either "yes" or "no",
but maybe the answer could be "yes", "no", or "look at this other
version in the UNDO segment instead".  The advantage of this approach
is that we don't end up copying tuples into a new "reconstructed"
page.  That copying will take CPU time and the resulting page will
consume cache space.

Now, the problem with this is that we want to avoid walking through
the whole UNDO chain for the page multiple times.  But maybe there are
ways to avoid that -- for starters, if the recently-modified bit on
the page isn't set, we don't need to walk through the UNDO chain at
all.  For a sequential scan, we could maintain a backend-local cache
of information that gets updated as we walk the chain.  Perhaps
there's no way to make these ideas work and we're going to end up
doing some form of page reconstruction, but I think it's worth some
thought anyway.

> Undo Page Format
> ---
> I think we can organize undo in multiple ways
>
> (a) One undo file per transaction - We can name each undo file as
> epoch+xid or epch+xid.undo. These files can be kept in pg_undo
> directory under PGDATA.  In this approach, we have to store
> latest_undo offset in file header to perform the rollback operation.
> Also, this offset can be used to allocate the location for new undo
> record.  Apart from that undo entries can be organized one after
> another.  Each undo entry will have two back pointers, one to reach
> the previous undo location which will be used for rollback and the
> second one to maintain the prior undo chain for changes to a
> particular page which will be used to reconstruct the copy of page.

Sounds good so far.

> During recovery,  we need to read each undo file and check the
> transaction status in 'clog', if it is not committed, then apply all
> the undo starting from latest_undo position.

I mostly agree, but let's make this a little more precise.  During
normal running, when a transaction aborts, we will perform all of the
undo actions associated with that transaction, and we will generate
WAL for those changes.  For example, if an insert adds a tuple to a
page, and the containing transaction aborts, then there will be an
undo entry to remove the tuple.  But removing that tuple has to be
WAL-logged, just as adding it was.  When all actions for the
transaction have been undone, we will remove the undo log for that
transaction and the 

Re: [HACKERS] pg_upgrade vs. pg_ctl stop -m fast

[Tom Lane]

Peter Eisentraut  writes:
> In 9.5, the default pg_ctl stop mode was changed from "smart" to "fast".
> In pg_upgrade, there is this code:
> ...
> I think the last line should be changed to something like
>   fast ? "-m fast" : "-m smart");

Ugh.  Clear oversight.

There is maybe room for a separate discussion about whether pg_upgrade
*should* be using fast mode, but if so we could remove the "bool fast"
argument from this function altogether.

regards, tom lane


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] pg_upgrade vs. pg_ctl stop -m fast

[Peter Eisentraut]

In 9.5, the default pg_ctl stop mode was changed from "smart" to "fast".

In pg_upgrade, there is this code:

void
stop_postmaster(bool fast)
{
...
exec_prog(SERVER_STOP_LOG_FILE, NULL, !fast,
  "\"%s/pg_ctl\" -w -D \"%s\" -o \"%s\" %s stop",
  cluster->bindir, cluster->pgconfig,
  cluster->pgopts ? cluster->pgopts : "",
  fast ? "-m fast" : "");
...
}

So, when upgrading from 9.5 or later, code that requested a non-fast
shutdown would now always get a fast shutdown.

I think the last line should be changed to something like

  fast ? "-m fast" : "-m smart");

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Retiring from the Core Team

[Jesper Pedersen]

On 01/11/2017 07:29 PM, Josh Berkus wrote:

You will have noticed that I haven't been very active for the past year.
 My new work on Linux containers and Kubernetes has been even more
absorbing than I anticipated, and I just haven't had a lot of time for
PostgreSQL work.

For that reason, as of today, I am stepping down from the PostgreSQL
Core Team.



Thanks for all your work over the years !

Best regards,
 Jesper



--
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] many copies of atooid() and oid_cmp()

[Tom Lane]

Peter Eisentraut  writes:
> On 1/11/17 11:25 PM, Tom Lane wrote:
>> +1 for the concept, but I'm a bit worried about putting atooid() in
>> postgres_ext.h.  That's going to impose on the namespace of libpq-using
>> applications, for instance.  A more conservative answer would be to
>> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
>> so I do see the consistency of adding this there.  Hard choice.

> How about two copies: one in postgres_fe.h and one in postgres.h?

That seems uglier than either of the other choices.

I don't personally have a huge problem with adding atooid in
postgres_ext.h, but I thought I'd better flag the potential issue
to see if anyone else thinks it's a big problem.

>> The oid_cmp() move looks fine if we only need it on the server side.
>> But doesn't pg_dump have one too?

> The pg_dump one isn't a qsort comparator, though.

OK.

regards, tom lane


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[Robert Haas]

On Thu, Jan 12, 2017 at 8:56 AM, David Steele  wrote:
>> So, to sum up things on this thread, here are the votes about the use
>> of aliases or a pure breakage:
>> - No to aliases, shake the world: Stephen, Tom, David F, Vik, Bruce M => 5
>> - Yes to aliases: Michael P, Andres, Peter E., Cynthia S, Jim N,
>> Vladimir, Simon R, Fujii-san => 8
>> If I misunderstood things, please feel free to speak up.
>
> I'm also in the "no to aliases" camp.

Me too.

I appreciate what people are saying about the pain that is involved
when we change administration interfaces.  However, I suspect that
most of that pain falls on tool authors, who are disproportionately
represented in this forum, and on DBAs, who may not like it much but
can probably cope with it.  It's not really going to affect
applications, because applications generally don't care about the
transaction log directly.  What is really bad is when we break syntax
that might be used in queries, or in PL/pgsql functions.  That's why
the 8.3 casting changes were so painful, and why the discussion about
changing PL/pgsql semantics is so dominated by people who don't want
to break what's worth now.  When we change things in those areas,
people who know nothing and care nothing about PostgreSQL other than
that it's the underlying database have to update their SQL, and that
hurts adoption.  Anyone who is affected by these proposed changes is
already deeply invested in PostgreSQL.

Also, I think it is absolutely wrong to suppose that there's no pain
involved in having these aliases around forever.  I thought the idea
of stuffing the aliases in a contrib module was a rather good one,
actually, because then the people who need them can install them
easily and yet the burden on core to maintain them is largely removed.
Also, there's still positive pressure for tools that haven't been
updated to get themselves updated to work without that compatibility
extension, so eventually the need for it should go away.  I have not
abandoned all hope of eventually removing contrib/tsearch2 from the
core distribution.  When we keep things in the core system catalogs,
they just accumulate forever.  There will not only be old code that
continues to use the old names; there will be new code that uses the
old names.  There will be people who ask why we have two copies of the
function, or aren't sure which one to use.  Somebody will have a
tab-completion fail that results from an irrelevant decision between
two names for the same thing.  Individually, these things don't matter
very much, but over time they lead to a user experience that's full of
warts and a developer experience that is enslaved to historical
garbage that takes time and energy away from new feature development.

So, my order of preference is:

1. Rename everything and add an extension to create aliases for the old names.
2. Rename everything without adding a backward-compatibility extension.
3. Don't rename anything.
4. Stomach flu.
5. Put a second name for everything in pg_proc.h.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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: pg_stat_statements query normalization issues with combined queries

[Tom Lane]

Kyotaro HORIGUCHI  writes:
> At Fri, 30 Dec 2016 15:10:42 +0100 (CET), Fabien COELHO  
> wrote in 
>> - I cannot use the intermediate node trick suggested by Tom because
>> it does not work for utility statements which do not have plans, so
>> the code still adds location & length, sorry.

I still really, really don't like this patch.  I think it's going to
create continuing maintenance problems, because of shortcuts like this:

+#define isParseNodeTag(tag)((T_Query <= (tag)) && ((tag) < T_A_Expr))

We've never had any code that depends on ranges of nodetag numbers, and
now is not the time to start.  (The fact that you had to randomly relocate
some existing tags to make this work didn't make me any happier about it.)

>> - I still use the 'last_semicolon' lexer variable. The alternative is to
>> change rules so as not to skip empty statements, then write a loop to
>> compute the length based on successor location, and remove the empty
>> statements. It can be done, I do not think it is better, it is only
>> different and more verbose. I'll do it if required by a committer.

> I think this is doable in the way shown in this patch. But this
> seems somewhat bogus, too..

Yeah, I doubt that this technique for getting the raw locations in the
grammar+lexer works reliably.  In general, Bison is a bit asynchronous
in terms of scanning, and may or may not have scanned a token ahead of
what the current production covers.  So having production rules that
look at the scanner state is just dangerous.  You should be relying on
the Bison locations mechanism (@N), instead.  That has some gotchas of
its own with respect to locations of nonterminals, but at least they're
known --- and we could fix them if we had to.


Anyway, I decided to see what it would take to do it the way I had
in mind, which was to stick a separate RawStmt node atop each statement
parsetree.  The project turned out a bit larger than I hoped :-(, but
I'm really pleased with the end result, because it makes the APIs around
lists of statements much simpler and more regular than they used to be.
I would probably propose most of the attached patch for adoption even
if we weren't interested in tracking statement boundaries.

In brief, what this does is:

* The output of raw parsing is now always a list of RawStmt nodes;
the statement-type-dependent nodes are one level down from that.
This is similar to the existing rule that the output of parse analysis
is always a list of Query nodes, even though the Query struct is mostly
empty in the CMD_UTILITY case.  Furthermore, the output of pg_plan_queries
is now always a list of PlannedStmt nodes, even for utility statements.
In the case of a utility statement, "planning" just consists of wrapping
a CMD_UTILITY PlannedStmt around the utility node.  Now, every list of
statements has a consistent head-node type depending on how far along
it is in processing.

* This allows us to keep statement location/length in RawStmt, Query,
and PlannedStmt nodes, and not anywhere else.

* This results in touching quite a bit of code that is concerned with
lists of statements in different formats, but in most places, it's
simple changes like replacing generic Node* pointers with specific
RawStmt* or PlannedStmt* pointers.  I think that's an unalloyed good.

* To preserve the new rule that the node passed to top-level
parse_analyze() is now always a RawStmt, I made the grammar insert
RawStmts in the sub-statements of EXPLAIN, DECLARE CURSOR, etc.
This adds a bit of cruft in gram.y but avoids cruft elsewhere.

* In addition, I had to change the API of ProcessUtility() so that
what it's passed is the wrapper PlannedStmt not just the bare
utility statement.  This is what allows pg_stat_statements to get at
the location info for a utility statement.  This will affect all
users of ProcessUtility_hook, but the changes are pretty trivial
for those that don't care about location info --- see the changes
to contrib/sepgsql/hooks.c for an example.

* Because PlannedStmt also carries a canSetTag field, we're able to
get rid of some ad-hoc rules about how to reconstruct canSetTag for
a bare utility statement (specifically, the assumption that a utility
is canSetTag if and only if it's the only one in its list).  While
I see no near-term need for relaxing that, it's nice to get rid of the
ad-hocery.

* I chose to also rearrange the post-parse-analysis representation
of DECLARE CURSOR so that it looks more like EXPLAIN, PREPARE, etc.
That is, the contained SELECT remains a child of the DeclareCursorStmt
rather than getting flipped around to be the other way.  Possibly this
patch could have been made to work without that, but the existing code
had some weird rules about how the presence of a PlannedStmt where a
utility command was expected meant it was DECLARE CURSOR, and I was out
to get rid of weird rules like that one.  With these changes, 

Re: [HACKERS] parallelize queries containing subplans

[Amit Kapila]

On Thu, Jan 12, 2017 at 8:51 AM, Robert Haas  wrote:
> On Wed, Jan 11, 2017 at 9:58 PM, Amit Kapila  wrote:
>> The other alternative is to remember this information in SubPlan.  We
>> can retrieve parallel_safe information from best_path and can use it
>> while generating SubPlan.  The main reason for storing it in the plan
>> was to avoid explicitly passing parallel_safe information while
>> generating SubPlan as plan was already available at that time.
>> However, it seems there are only two places in code (refer
>> build_subplan) where this information needs to be propagated.  Let me
>> know if you prefer to remember the parallel_safe information in
>> SubPlan instead of in Plan or if you have something else in mind?
>
> I think we should try doing it in the SubPlan, at least, and see if
> that comes out more elegant than what you have at the moment.
>

Okay, done that way.  I have fixed the review comments raised by Dilip
as well and added the test case in attached patch.

-- 
With Regards,
Amit Kapila.
EnterpriseDB: http://www.enterprisedb.com


pq_pushdown_subplan_v2.patch
Description: Binary data

-- 
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] [PATCH] Fix minor race in commit_ts SLRU truncation vs lookups

[Peter Eisentraut]

On 12/29/16 4:28 AM, Craig Ringer wrote:
> On 29 December 2016 at 16:51, Craig Ringer  wrote:
>> On 28 December 2016 at 22:16, Petr Jelinek  
>> wrote:
>>
>>> About the patch, it looks good to me for master with the minor exception
>>> that:
 + appendStringInfo(buf, "pageno %d, xid %u",
 + trunc.pageno, trunc.oldestXid);
>>>
>>> This should probably say oldestXid instead of xid in the text description.
>>
>> Agreed.
> 
> Slightly amended attached.

I've looked over this.  It looks correct to me in principle.

The commit message does not actually explain how the race on the standby
is fixed by the patch.

Also, I wonder whether we should not in vacuum.c change the order of the
calls of SetTransactionIdLimit() and SetMultiXactIdLimit() as well, just
to keep everything consistent.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Obsolete reference to ParallelMain() in a comment

[Robert Haas]

On Wed, Jan 11, 2017 at 2:38 AM, Amit Langote
 wrote:
> The comment above ParallelQueryMain() still refers to ParallelMain() as
> its caller which is no longer the case.  Attached fixes that.

Oops.

Thanks, committed.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
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] [PATCH] Rename pg_switch_xlog to pg_switch_wal

[David Steele]

On 1/12/17 12:49 AM, Michael Paquier wrote:
> On Thu, Jan 12, 2017 at 2:00 AM, Vladimir Rusinov  wrote:
>> On Tue, Jan 10, 2017 at 5:24 AM, Michael Paquier 
>> wrote:
>>> As there are two school of thoughts on this thread, keeping your patch
>>> with the compatibility table is the best move for now. Even if we end
>>> up by having a version without aliases, that will be just code to
>>> remove in the final version.
>>
>> Indeed, it is trivial to kill aliases.
>>
>> New version of the patch attached.
> 
> The patch still updates a bunch of .po files. Those are normally
> refreshed with the translation updates, so they had better be removed.
> Other than that, the patch looks in good shape to me so switch to
> "Ready for committer".
> 
> So, to sum up things on this thread, here are the votes about the use
> of aliases or a pure breakage:
> - No to aliases, shake the world: Stephen, Tom, David F, Vik, Bruce M => 5
> - Yes to aliases: Michael P, Andres, Peter E., Cynthia S, Jim N,
> Vladimir, Simon R, Fujii-san => 8
> If I misunderstood things, please feel free to speak up.

I'm also in the "no to aliases" camp.

-- 
-David
da...@pgmasters.net


-- 
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] many copies of atooid() and oid_cmp()

[Peter Eisentraut]

On 1/12/17 12:25 AM, Kuntal Ghosh wrote:
> On Wed, Jan 11, 2017 at 9:42 PM, Peter Eisentraut
>  wrote:
>> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
>> equivalent, and pending patches are proposing to add more.  I propose
>> these two patches to collect them in central places.
>>
> I've verified that the patch covers all the copies of atooid() and
> oid_cmp() that should be replaced. However, as Tom suggested, I've
> looked into pg_dump and found that there is a oidcmp() as well. It may
> have been missed because it has a different name.
> 
> I was wondering whether it is worth to replace the following as well:
> (TransactionId)strtoul(str, NULL, 16) to something like #define atotranid().

There is a atoxid(), which is actually not used and removed by my patch.
 There are a few other calls of this pattern, but they are not frequent
or consistent enough to worry about (yet), I think.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] many copies of atooid() and oid_cmp()

[Peter Eisentraut]

On 1/11/17 11:25 PM, Tom Lane wrote:
> Peter Eisentraut  writes:
>> There are approximately 11 copies of atooid() and 3 of oid_cmp() or
>> equivalent, and pending patches are proposing to add more.  I propose
>> these two patches to collect them in central places.
> 
> +1 for the concept, but I'm a bit worried about putting atooid() in
> postgres_ext.h.  That's going to impose on the namespace of libpq-using
> applications, for instance.  A more conservative answer would be to
> add it to c.h.  OTOH, postgres_ext.h is where the Oid typedef lives,
> so I do see the consistency of adding this there.  Hard choice.

How about two copies: one in postgres_fe.h and one in postgres.h?

> The oid_cmp() move looks fine if we only need it on the server side.
> But doesn't pg_dump have one too?

The pg_dump one isn't a qsort comparator, though.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Proposal for changes to recovery.conf API

[Peter Eisentraut]

On 1/11/17 5:27 AM, Simon Riggs wrote:
> The main area of "design doubt" remains the implementation of the
> recovery_target parameter set. Are we happy with the user interface
> choices in the patch, given the understanding that the situation was
> more comple than at first thought?

Could you summarize the current proposal(s)?

Personally, I don't immediately see the need to change anything from the
parameter names that I currently see in recovery.conf.sample.

-- 
Peter Eisentraut  http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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: pg_stat_statements query normalization issues with combined queries

[Kyotaro HORIGUCHI]

Hello,

At Fri, 30 Dec 2016 15:10:42 +0100 (CET), Fabien COELHO  
wrote in 
> 
> Hello,
> 
> >> Yeah, that's what I was thinking of.  There aren't very many places
> >> that
> >> would need to know about that, I believe; [...]
> >
> > For fixing the information in pg_stat_statement, the location data
> > must be transported from the parsed node to the query to the planned
> > node, because the later two nodes types are passed to different hooks.
> >
> > Now the detail is that utility statements, which seems to be nearly
> > all of them but select/update/delete/insert, do not have plans: The
> > statement itself is its own plan... so there is no place to store the
> > location & length.
> 
> Here is an updated version:
> 
> Changes wrt v2:
> 
>  - I have added the missing stuff under /nodes/, this is stupid code that
>should be automatically generated:-(
>
>  - I have added comments in "gram.y" about how the length is computed.
>I have also slightly simplified the rule code there.
> 
>  - I have rename "location" in create table space to "location_dir"
>to avoid confusion.
> 
>  - I have renamed the fields "location" and "length" instead of q*.
> 
>  - I have moved the location & lenth copies in standard_planner.
> 
>  - I have fixed the function declaration typo.
> 
>  - I have simplified pgss_store code to avoid a copy, and move the
>length truncation in qtext_store.
> 
>  - I have improved again the pg_stat_statement regression tests with
>combined utility statement tests, which implied some fixes to
>extract the right substring for utility queries.
> 
> However, not changed:
> 
>  - I cannot use the intermediate node trick suggested by Tom because
>it does not work for utility statements which do not have plans, so
>the code still adds location & length, sorry.

Wrapping the output of pg_parse_query still works for non-utility
commands. But pg_stat_statement retrieves the location
information via ProcessUtility, which has plantree. Wrapping
plantree with the same struct also works but it imacts too widely
and extremely error-prone.

One available solution is, as Fabien did, bring location and
length data along with transformation.  And anther is give a
chopped query to query tree.

The attached patch does the second way (but quite ugly and
incomplete, it's PoC). This seems working as expected to a
certain degree but somewhat bogus..  A significant drawback of
this is that this makes a copy of each query in multistatement.

>  - I still use the 'last_semicolon' lexer variable. The alternative is to
>change rules so as not to skip empty statements, then write a loop to
>compute the length based on successor location, and remove the empty
>statements. It can be done, I do not think it is better, it is only
>different and more verbose. I'll do it if required by a committer.

I think this is doable in the way shown in this patch. But this
seems somewhat bogus, too..

regards,

-- 
Kyotaro Horiguchi
NTT Open Source Software Center
>From 20b34be19e51b878c33306d13a453bdd05da23bc Mon Sep 17 00:00:00 2001
From: Kyotaro Horiguchi 
Date: Thu, 12 Jan 2017 21:51:34 +0900
Subject: [PATCH 1/2] Fix current query string of query tree.

---
 src/backend/catalog/pg_proc.c  |  2 +-
 src/backend/commands/extension.c   |  2 +-
 src/backend/commands/foreigncmds.c |  3 ++-
 src/backend/commands/prepare.c |  2 +-
 src/backend/commands/tablecmds.c   |  2 +-
 src/backend/executor/functions.c   |  2 +-
 src/backend/executor/spi.c |  4 ++--
 src/backend/nodes/copyfuncs.c  | 15 +++
 src/backend/nodes/equalfuncs.c | 13 +
 src/backend/parser/analyze.c   |  4 
 src/backend/parser/gram.y  | 20 ++--
 src/backend/parser/parse_type.c|  2 +-
 src/backend/tcop/postgres.c| 22 --
 src/backend/tcop/pquery.c  |  1 +
 src/include/nodes/nodes.h  |  1 +
 src/include/nodes/parsenodes.h | 20 
 src/include/nodes/plannodes.h  |  2 ++
 17 files changed, 100 insertions(+), 17 deletions(-)

diff --git a/src/backend/catalog/pg_proc.c b/src/backend/catalog/pg_proc.c
index 6d8f17d..6d9e639 100644
--- a/src/backend/catalog/pg_proc.c
+++ b/src/backend/catalog/pg_proc.c
@@ -934,7 +934,7 @@ fmgr_sql_validator(PG_FUNCTION_ARGS)
 			querytree_list = NIL;
 			foreach(lc, raw_parsetree_list)
 			{
-Node	   *parsetree = (Node *) lfirst(lc);
+Node	   *parsetree = (Node *) stripParseNode(lfirst(lc));
 List	   *querytree_sublist;
 
 querytree_sublist = pg_analyze_and_rewrite_params(parsetree,
diff --git a/src/backend/commands/extension.c b/src/backend/commands/extension.c
index be52148..36ebf93 100644
--- a/src/backend/commands/extension.c
+++ b/src/backend/commands/extension.c
@@ -712,7 +712,7 @@ execute_sql_string(const char *sql, const char *filename)
 	 

Re: [HACKERS] Unused member root in foreign_glob_cxt

[Tom Lane]

Ashutosh Bapat  writes:
> The member root in foreign_glob_cxt isn't used anywhere by
> postgres_fdw code. Without that member the code compiles and
> regression passes. The member was added by d0d75c40. I looked at that
> commit briefly but did not find any code using it there. So, possibly
> it's unused since it was introduced. Should we drop that member?

I think you'd just end up putting it back at some point.  It's the only
means that foreign_expr_walker() has for getting at the root pointer,
and nearly all planner code needs that.  Seems to me it's just chance
that foreign_expr_walker() doesn't need it right now.

> PFA the patch to remove that member. If we decide to drop that member,
> we can drop root argument to is_foreign_expr() and clean up some more
> code. I volunteer to do that, if we agree.

That would *really* be doubling down on the assumption that
is_foreign_expr() will never ever need access to the root.

regards, tom lane


-- 
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] Retiring from the Core Team

[Andrew Dunstan]

On 01/11/2017 07:29 PM, Josh Berkus wrote:

Hackers:

You will have noticed that I haven't been very active for the past year.
  My new work on Linux containers and Kubernetes has been even more
absorbing than I anticipated, and I just haven't had a lot of time for
PostgreSQL work.

For that reason, as of today, I am stepping down from the PostgreSQL
Core Team.




Josh,

I have valued you as a community member, as a work colleague, and not 
least as a friend. Your absence will be sad, you presence still very 
welcome.


cheers

andrew

--
Andrew Dunstanhttps://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services



--
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] Patch to implement pg_current_logfile() function

[Karl O. Pinc]

On Thu, 12 Jan 2017 13:14:28 +0100
Gilles Darold  wrote:

> My bad, I was thinking that Karl has planned an update of the patch in
> his last post, sorry for my misunderstanding.

I was, but have been swept along by events and not gotten to it.



Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein


-- 
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] Passing query string to workers

[Robert Haas]

On Wed, Jan 11, 2017 at 11:12 PM, Tom Lane  wrote:
> Robert Haas  writes:
>> On Wed, Jan 11, 2017 at 6:36 PM, Tom Lane  wrote:
>>> That would work, if you had a way to get at the active QueryDesc ...
>>> but we don't pass that down to executor nodes.
>
>> Hmm, that is a bit of a problem.  Do you have a suggestion?
>
> Copy that string pointer into the EState, perhaps?

OK, that sounds good.

-- 
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

[HACKERS] Unused member root in foreign_glob_cxt

[Ashutosh Bapat]

Hi,
The member root in foreign_glob_cxt isn't used anywhere by
postgres_fdw code. Without that member the code compiles and
regression passes. The member was added by d0d75c40. I looked at that
commit briefly but did not find any code using it there. So, possibly
it's unused since it was introduced. Should we drop that member?

PFA the patch to remove that member. If we decide to drop that member,
we can drop root argument to is_foreign_expr() and clean up some more
code. I volunteer to do that, if we agree.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


pgfdw_unused_root.patch
Description: fcatjava/download

-- 
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] Parallel bitmap heap scan

[Dilip Kumar]

On Thu, Jan 12, 2017 at 5:22 PM, Rafia Sabih
 wrote:
> Hello Dilip,
> I was trying to test the performance of all the parallel query related
> patches on TPC-H queries, I found that when parallel hash [1 ]and your
> patch are applied then Q10 and Q14 were hanged, however, without any
> one of these patches, these queries are running fine. Following is the
> stack trace,

Thanks for reporting, I will look into this.


-- 
Regards,
Dilip Kumar
EnterpriseDB: http://www.enterprisedb.com


-- 
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] Patch to implement pg_current_logfile() function

[Gilles Darold]

Le 11/01/2017 à 08:39, Michael Paquier a écrit :
> On Wed, Dec 14, 2016 at 12:19 AM, Robert Haas  wrote:
>> On Mon, Dec 12, 2016 at 4:31 AM, Gilles Darold  
>> wrote:
>>> Applied in last version of the patch v18 as well as removing of an
>>> unused variable.
>> OK, this looks a lot closer to being committable.  But:
>>
>> [long review]
>  
> Gilles, could you update the patch based on this review from Robert? I
> am marking the patch as "waiting on author" for the time being. I
> looked a bit at the patch but did not notice anything wrong with it,
> but let's see with a fresh version...
Hi,

My bad, I was thinking that Karl has planned an update of the patch in
his last post, sorry for my misunderstanding.

Here is attached v19 of the patch that might fix all comment from
Robert's last review.

Best regards,

-- 
Gilles Darold
Consultant PostgreSQL
http://dalibo.com - http://dalibo.org

diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml
index 30dd54c..dd23932 100644
--- a/doc/src/sgml/config.sgml
+++ b/doc/src/sgml/config.sgml
@@ -4211,6 +4211,14 @@ SELECT * FROM parent WHERE key = 2400;
  server log
 
 
+
+When logs are written to the file-system their paths, names, and
+types are recorded in
+the  file.  This provides
+a convenient way to find and access log content without establishing a
+database connection.
+
+
 
  Where To Log
 
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 10e3186..e4723f4 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -15441,6 +15441,19 @@ SELECT * FROM pg_ls_dir('.') WITH ORDINALITY AS t(ls,n);
   
 
   
+   pg_current_logfile()
+   text
+   primary log file name in use by the logging collector
+  
+
+  
+   pg_current_logfile(text)
+   text
+   log file name, of log in the requested format, in use by the
+   logging collector
+  
+
+  
pg_my_temp_schema()
oid
OID of session's temporary schema, or 0 if none
@@ -15658,6 +15671,39 @@ SET search_path TO schema , schema, ..
 the time when the postmaster process re-read the configuration files.)

 
+
+pg_current_logile
+   
+
+   
+Logging
+pg_current_logfile function
+   
+
+   
+pg_current_logfile returns, as text,
+the path of either the csv or stderr log file currently in use by the
+logging collector.  This is a path including the
+ directory and the log file name.
+Log collection must be active or the return value
+is NULL.  When multiple logfiles exist, each in a
+different format, pg_current_logfile called
+without arguments returns the path of the file having the first format
+found in the ordered
+list: stderr, csvlog.
+NULL is returned when no log file has any of these
+formats.  To request a specific file format supply,
+as text, either csvlog
+or stderr as the value of the optional parameter. The
+return value is NULL when the log format requested
+is not a configured .
+
+pg_current_logfiles reflects the content of the
+ file.  All caveats
+regards current_logfiles content are applicable
+to pg_current_logfiles' return value.
+   
+

 pg_my_temp_schema

diff --git a/doc/src/sgml/storage.sgml b/doc/src/sgml/storage.sgml
index 5c52824..3ce2a7e 100644
--- a/doc/src/sgml/storage.sgml
+++ b/doc/src/sgml/storage.sgml
@@ -60,6 +60,38 @@ Item
  Subdirectory containing per-database subdirectories
 
 
+
+ 
+  
+   current_logfiles
+  
+  
+   Logging
+   current_logfiles file
+  
+  current_logfiles
+ 
+ 
+  
+  A file recording the log file(s) currently written to by the syslogger
+  and the file's log formats, stderr
+  or csvlog.  Each line of the file is a space separated list of
+  two elements: the log format and the full path to the log file including the
+  value of .  The log format must be present
+  in  to be listed in
+  current_logfiles.
+  
+
+  
+  The current_logfiles file
+  is present only when  is
+  activated and when at least one of stderr or
+  csvlog value is present in
+  .
+  
+ 
+
+
 
  global
  Subdirectory containing cluster-wide tables, such as
diff --git a/src/backend/postmaster/syslogger.c b/src/backend/postmaster/syslogger.c
index 13a0301..ecaeeef 100644
--- a/src/backend/postmaster/syslogger.c
+++ b/src/backend/postmaster/syslogger.c
@@ -146,7 +146,7 @@ static char *logfile_getname(pg_time_t timestamp, const char *suffix);
 static void set_next_rotation_time(void);
 static void sigHupHandler(SIGNAL_ARGS);
 static void sigUsr1Handler(SIGNAL_ARGS);
-
+static void update_metainfo_datafile(void);
 
 /*
  * Main entry point for syslogger process
@@ -348,6 +348,13 @@ SysLoggerMain(int argc, char *argv[])
 rotation_disabled = false;
 rotation_requested = true;
 			}
+			/*
+			 * Force rewriting last log filename when reloading configuration,
+			 * even if 

Re: [HACKERS] Parallel Index-only scan

[Rahila Syed]

Hello,

On applying the patch on latest master branch and running regression tests
following failure occurs.
I applied it on latest parallel index scan patches as given in the link
above.

***
/home/rahila/postgres/postgres/src/test/regress/expected/select_parallel.out
2017-01-03 14:06:29.122022780 +0530
---
/home/rahila/postgres/postgres/src/test/regress/results/select_parallel.out
2017-01-12 14:35:56.652712622 +0530
***
*** 92,103 
  explain (costs off)
select  sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
!  QUERY PLAN
! 
   HashAggregate
 Group Key: parallel_restricted(unique1)
!->  Index Only Scan using tenk1_unique1 on tenk1
! (3 rows)

  set force_parallel_mode=1;
  explain (costs off)
--- 92,105 
  explain (costs off)
select  sum(parallel_restricted(unique1)) from tenk1
group by(parallel_restricted(unique1));
! QUERY PLAN
! ---
   HashAggregate
 Group Key: parallel_restricted(unique1)
!->  Gather
!  Workers Planned: 4
!  ->  Parallel Index Only Scan using tenk1_unique1 on tenk1
! (5 rows)

IIUC, parallel operation being performed here is fine as parallel
restricted function occurs above Gather node
and just the expected output needs to be changed.

Thank you,
Rahila Syed


regression.diffs
Description: Binary data

-- 
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] Parallel bitmap heap scan

[Rafia Sabih]

On Wed, Jan 11, 2017 at 5:33 PM, tushar  wrote:
>
> On 01/10/2017 05:16 PM, Dilip Kumar wrote:
>>
>>   Please try attached patch and confirm from your
>> side.
>
> Thanks,issue seems to be fixed now.
>
>
> --
> regards,tushar
> EnterpriseDB  https://www.enterprisedb.com/
> The Enterprise PostgreSQL Company
>
>
>
> --
> Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
> To make changes to your subscription:
> http://www.postgresql.org/mailpref/pgsql-hackers


Hello Dilip,
I was trying to test the performance of all the parallel query related
patches on TPC-H queries, I found that when parallel hash [1 ]and your
patch are applied then Q10 and Q14 were hanged, however, without any
one of these patches, these queries are running fine. Following is the
stack trace,

At the master:

#0  0x3fff8bef7de8 in __epoll_wait_nocancel () from /lib64/power8/libc.so.6
#1  0x104ea184 in WaitEventSetWaitBlock (set=0x10039b2d360,
cur_timeout=-1, occurred_events=0x3fffd4a55a98, nevents=1) at
latch.c:998
#2  0x104e9fc8 in WaitEventSetWait (set=0x10039b2d360,
timeout=-1, occurred_events=0x3fffd4a55a98, nevents=1,
wait_event_info=134217730) at latch.c:950
#3  0x104e9484 in WaitLatchOrSocket (latch=0x3fff85128ab4,
wakeEvents=1, sock=-1, timeout=-1, wait_event_info=134217730) at
latch.c:350
#4  0x104e931c in WaitLatch (latch=0x3fff85128ab4,
wakeEvents=1, timeout=0, wait_event_info=134217730) at latch.c:304
#5  0x1032a378 in gather_readnext (gatherstate=0x10039aeeb98)
at nodeGather.c:393
#6  0x1032a044 in gather_getnext (gatherstate=0x10039aeeb98)
at nodeGather.c:293
#7  0x10329e94 in ExecGather (node=0x10039aeeb98) at nodeGather.c:234
#8  0x103086f0 in ExecProcNode (node=0x10039aeeb98) at
execProcnode.c:521
#9  0x1031f550 in fetch_input_tuple (aggstate=0x10039aed220)
at nodeAgg.c:587
#10 0x103229a4 in agg_retrieve_direct (aggstate=0x10039aed220)
at nodeAgg.c:2211

At one of the worker process,

#0  0x103879f8 in pagetable_insert (tb=0x10039abcac0,
key=6491, found=0x3fffd4a54b88 "") at
../../../src/include/lib/simplehash.h:571
#1  0x10389964 in tbm_get_pageentry (tbm=0x10039ab5778,
pageno=6491) at tidbitmap.c:876
#2  0x10388608 in tbm_add_tuples (tbm=0x10039ab5778,
tids=0x10039a94c30, ntids=1, recheck=0 '\000') at tidbitmap.c:340
#3  0x100f5e54 in btgetbitmap (scan=0x10039ab3c80,
tbm=0x10039ab5778) at nbtree.c:439
#4  0x100eab7c in index_getbitmap (scan=0x10039ab3c80,
bitmap=0x10039ab5778) at indexam.c:687
#5  0x10328acc in MultiExecBitmapIndexScan
(node=0x10039a98630) at nodeBitmapIndexscan.c:98
#6  0x103088d8 in MultiExecProcNode (node=0x10039a98630) at
execProcnode.c:591
#7  0x10326c70 in BitmapHeapNext (node=0x10039a98770) at
nodeBitmapHeapscan.c:164
#8  0x10316440 in ExecScanFetch (node=0x10039a98770,
accessMtd=0x10326b70 , recheckMtd=0x10327bb4
) at execScan.c:95
#9  0x10316590 in ExecScan (node=0x10039a98770,
accessMtd=0x10326b70 , recheckMtd=0x10327bb4
) at execScan.c:180
#10 0x10327c84 in ExecBitmapHeapScan (node=0x10039a98770) at
nodeBitmapHeapscan.c:623

At other workers,

#0  0x3fff8bef7de8 in __epoll_wait_nocancel () from /lib64/power8/libc.so.6
#1  0x104ea184 in WaitEventSetWaitBlock (set=0x10039a24670,
cur_timeout=-1, occurred_events=0x3fffd4a55ed8, nevents=1) at
latch.c:998
#2  0x104e9fc8 in WaitEventSetWait (set=0x10039a24670,
timeout=-1, occurred_events=0x3fffd4a55ed8, nevents=1,
wait_event_info=134217737) at latch.c:950
#3  0x1051a3dc in ConditionVariableSleep (cv=0x3fff7c2d01ac,
wait_event_info=134217737) at condition_variable.c:132
#4  0x103283e0 in pbms_is_leader (pbminfo=0x3fff7c2d0178) at
nodeBitmapHeapscan.c:900
#5  0x10326c38 in BitmapHeapNext (node=0x10039a95f50) at
nodeBitmapHeapscan.c:153
#6  0x10316440 in ExecScanFetch (node=0x10039a95f50,
accessMtd=0x10326b70 , recheckMtd=0x10327bb4
) at execScan.c:95
#7  0x10316590 in ExecScan (node=0x10039a95f50,
accessMtd=0x10326b70 , recheckMtd=0x10327bb4
) at execScan.c:180
#8  0x10327c84 in ExecBitmapHeapScan (node=0x10039a95f50) at
nodeBitmapHeapscan.c:623
#9  0x10308588 in ExecProcNode (node=0x10039a95f50) at
execProcnode.c:443
#10 0x103310d8 in ExecHashJoinOuterGetTuple
(outerNode=0x10039a95f50, hjstate=0x10039a96808,
hashvalue=0x3fffd4a5639c) at nodeHashjoin.c:936

I was using TPC-H with scale factor 20, please let me know if there is
anything more you require in this regard.

[1] 
https://www.postgresql.org/message-id/CAEepm%3D1vGcv6LBrxZeqPb_rPxfraidWAF_8_4z2ZMQ%2B7DOjj9w%40mail.gmail.com
-- 
Regards,
Rafia Sabih
EnterpriseDB: http://www.enterprisedb.com/


-- 
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] Retiring from the Core Team

[Masahiko Sawada]

On Thu, Jan 12, 2017 at 9:29 AM, Josh Berkus  wrote:
> Hackers:
>
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.
>

Thank you for all your efforts!

Regards,

--
Masahiko Sawada
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center


-- 
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] Transactions involving multiple postgres foreign servers

[Masahiko Sawada]

On Fri, Dec 23, 2016 at 1:49 AM, Masahiko Sawada  wrote:
> On Fri, Dec 9, 2016 at 4:02 PM, Masahiko Sawada  wrote:
>> On Fri, Dec 9, 2016 at 3:02 PM, vinayak  
>> wrote:
>>> On 2016/12/05 14:42, Ashutosh Bapat wrote:

 On Mon, Dec 5, 2016 at 11:04 AM, Haribabu Kommi
  wrote:


 On Fri, Nov 11, 2016 at 5:38 PM, Masahiko Sawada 
 wrote:
>>
>>
>> 2PC is a basic building block to support the atomic commit and there
>> are some optimizations way in order to reduce disadvantage of 2PC. As
>> you mentioned, it's hard to support a single model that would suit
>> several type of FDWs. But even if it's not a purpose for sharding,
>> because many other database which could be connected to PostgreSQL via
>> FDW supports 2PC, 2PC for FDW would be useful for not only sharding
>> purpose. That's why I was focusing on implementing 2PC for FDW so far.
>
>
> Moved to next CF with "needs review" status.

 I think this should be changed to "returned with feedback.". The
 design and approach itself needs to be discussed. I think, we should
 let authors decide whether they want it to be added to the next
 commitfest or not.

 When I first started with this work, Tom had suggested me to try to
 make PREPARE and COMMIT/ROLLBACK PREPARED involving foreign servers or
 at least postgres_fdw servers work. I think, most of my work that
 Vinayak and Sawada have rebased to the latest master will be required
 for getting what Tom suggested done. We wouldn't need a lot of changes
 to that design. PREPARE involving foreign servers errors out right
 now. If we start supporting prepared transactions involving foreign
 servers that will be a good improvement over the current status-quo.
 Once we get that done, we can continue working on the larger problem
 of supporting ACID transactions involving foreign servers.
>>>
>>> In the pgconf ASIA depelopers meeting Bruce Momjian and other developers
>>> discussed
>>> on FDW based sharding [1]. The suggestions from other hackers was that we
>>> need to discuss
>>> the big picture and use cases of sharding. Bruce has listed all the building
>>> blocks of built-in sharding
>>> on wiki [2]. IIUC,transaction manager involving foreign servers is one part
>>> of sharding.
>>
>> Yeah, the 2PC on FDW is a basic building block for FDW based sharding
>> and it would be useful not only FDW sharding but also other purposes.
>> As far as I surveyed some papers the many kinds of distributed
>> transaction management architectures use the 2PC for atomic commit
>> with some optimisations. And using 2PC to provide atomic commit on
>> distributed transaction has much affinity with current PostgreSQL
>> implementation from some perspective.
>>
>>> As per the Bruce's wiki page there are two use cases for transactions
>>> involved multiple foreign servers:
>>> 1. Cross-node read-only queries on read/write shards:
>>> This will require a global snapshot manager to make sure the shards
>>> return consistent data.
>>> 2. Cross-node read-write queries:
>>> This will require a global snapshot manager and global transaction
>>> manager.
>>>
>>> I agree with you that if we start supporting PREPARE and COMMIT/ROLLBACK
>>> PREPARED
>>> involving foreign servers that will be good improvement.
>>>
>>> [1] https://wiki.postgresql.org/wiki/PgConf.Asia_2016_Developer_Meeting
>>> [2] https://wiki.postgresql.org/wiki/Built-in_Sharding
>>>
>>
>> I also agree to work on implementing the atomic commit across the
>> foreign servers and then continue to work on the more larger problem.
>> I think that this will be large step forward. I'm going to submit the
>> updated version patch to CF3.
>
> Attached latest version patches. Almost design is the same as previous
> patches and I incorporated some optimisations and updated
> documentation. But the documentation and regression test is not still
> enough.
>
> 000 patch adds some new FDW APIs to achive the atomic commit involving
> the foreign servers using two-phase-commit. If more than one foreign
> servers involve with the transaction or the transaction changes local
> data and involves even one foreign server, local node executes PREPARE
> and COMMIT/ROLLBACK PREPARED on foreign servers at commit. A lot of
> part of this implementation is inspired by two phase commit code. So I
> incorporated recent changes of two phase commit code, for example
> recovery speed improvement, into this patch.
> 001 patch makes postgres_fdw support atomic commit. If
> two_phase_commit is set 'on' to a foreign server, the two-phase-commit
> will be used at commit. 002 patch adds the pg_fdw_resolver new contrib
> module that is a bgworker process that resolves the in-doubt
> transaction on foreign server if there is.
>
> The reply might be 

Re: [HACKERS] WIP: [[Parallel] Shared] Hash

[Rafia Sabih]

On Thu, Jan 12, 2017 at 9:07 AM, Thomas Munro  wrote:

> On Wed, Jan 11, 2017 at 2:56 PM, Peter Geoghegan  wrote:
> > On Fri, Jan 6, 2017 at 12:01 PM, Thomas Munro
> >  wrote:
> >> Here is a new WIP patch.  I have plenty of things to tidy up (see note
> >> at end), but the main ideas are now pretty clear and I'd appreciate
> >> some feedback.
> >
> > I have some review feedback for your V3. I've chosen to start with the
> > buffile.c stuff, since of course it might share something with my
> > parallel tuplesort patch. This isn't comprehensive, but I will have
> > more comprehensive feedback soon.
>
> Thanks!
>
> > I'm not surprised that you've generally chosen to make shared BufFile
> > management as simple as possible, with no special infrastructure other
> > than the ability to hold open other backend temp files concurrently
> > within a worker, and no writing to another worker's temp file, or
> > shared read pointer. As you put it, everything is immutable. I
> > couldn't see much opportunity for adding a lot of infrastructure that
> > wasn't written explicitly as parallel hash join code/infrastructure.
> > My sense is that that was a good decision. I doubted that you'd ever
> > want some advanced, generic shared BufFile thing with multiple read
> > pointers, built-in cache coherency, etc. (Robert seemed to think that
> > you'd go that way, though.)
>
> Right, this is extremely minimalist infrastructure.  fd.c is
> unchanged.  buffile.c only gains the power to export/import read-only
> views of BufFiles.  There is no 'unification' of BufFiles: each hash
> join participant simply reads from the buffile it wrote, and then
> imports and reads from its peers' BufFiles, until all are exhausted;
> so the 'unification' is happening in caller code which knows about the
> set of participants and manages shared read positions.  Clearly there
> are some ownership/cleanup issues to straighten out, but I think those
> problems are fixable (probably involving refcounts).
>
> I'm entirely willing to throw that away and use the unified BufFile
> concept, if it can be extended to support multiple readers of the
> data, where every participant unifies the set of files.  I have so far
> assumed that it would be most efficient for each participant to read
> from the file that it wrote before trying to read from files written
> by other participants.  I'm reading your patch now; more soon.
>
> > Anyway, some more specific observations:
> >
> > * ISTM that this is the wrong thing for shared BufFiles:
> >
> >> +BufFile *
> >> +BufFileImport(BufFileDescriptor *descriptor)
> >> +{
> > ...
> >> +   file->isInterXact = true; /* prevent cleanup by this backend */
> >
> > There is only one user of isInterXact = true BufFiles at present,
> > tuplestore.c. It, in turn, only does so for cases that require
> > persistent tuple stores. A quick audit of these tuplestore.c callers
> > show this to just be cursor support code within portalmem.c. Here is
> > the relevant tuplestore_begin_heap() rule that that code adheres to,
> > unlike the code I've quoted above:
> >
> >  * interXact: if true, the files used for on-disk storage persist beyond
> the
> >  * end of the current transaction.  NOTE: It's the caller's
> responsibility to
> >  * create such a tuplestore in a memory context and resource owner that
> will
> >  * also survive transaction boundaries, and to ensure the tuplestore is
> closed
> >  * when it's no longer wanted.
>
> Hmm.  Yes, that is an entirely bogus use of isInterXact.  I am
> thinking about how to fix that with refcounts.
>
> > I don't think it's right for buffile.c to know anything about file
> > paths directly -- I'd say that that's a modularity violation.
> > PathNameOpenFile() is called by very few callers at the moment, all of
> > them very low level (e.g. md.c), but you're using it within buffile.c
> > to open a path to the file that you obtain from shared memory
>
> Hmm.  I'm not seeing the modularity violation.  buffile.c uses
> interfaces already exposed by fd.c to do this:  OpenTemporaryFile,
> then FilePathName to find the path, then PathNameOpenFile to open from
> another process.  I see that your approach instead has client code
> provide more meta data so that things can be discovered, which may
> well be a much better idea.
>
> > directly. This is buggy because the following code won't be reached in
> > workers that call your BufFileImport() function:
> >
> > /* Mark it for deletion at close */
> > VfdCache[file].fdstate |= FD_TEMPORARY;
> >
> > /* Register it with the current resource owner */
> > if (!interXact)
> > {
> > VfdCache[file].fdstate |= FD_XACT_TEMPORARY;
> >
> > ResourceOwnerEnlargeFiles(CurrentResourceOwner);
> > ResourceOwnerRememberFile(CurrentResourceOwner, file);
> > VfdCache[file].resowner = CurrentResourceOwner;
> >
> > /* ensure cleanup happens 

Re: [HACKERS] Proposal for changes to recovery.conf API

[Simon Riggs]

On 12 January 2017 at 05:49, Fujii Masao  wrote:
> On Wed, Jan 11, 2017 at 7:36 PM, Simon Riggs  wrote:
>> On 11 January 2017 at 09:51, Fujii Masao  wrote:
>>
 5. recovery.conf parameters are all moved to postgresql.conf, with these 
 changes
>>>
>>> In current design of the patch, when recovery parameters are misconfigured
>>> (e.g., set recovery_target_timeline to invalid timeline id) and
>>> the configuration file is reloaded, the startup process emits FATAL error 
>>> and
>>> the server goes down. I don't think this is fine. Basically even
>>> misconfiguration of the parameters should not cause the server crash.
>>> If invalid settings are supplied, I think that we just should warn them
>>> and ignore those new settings, like current other GUC is. Thought?
>>
>> Thanks for your comments.
>>
>> The specification of the recovery target parameters should be different, 
>> IMHO.
>>
>> If the user is performing a recovery and the target of the recovery is
>> incorrectly specified then it is clear that the recovery cannot
>> continue with an imprecisely specified target.
>
> Could you tell me what case of "the target of the recovery is incorrectly
> specified" are you thinking of? For example, you're thinking the case where
> invalid format of timestamp value is specified in recovery_target_time?
> In this case, I think that we should issue a WARNING and ignore that invalid
> setting when the configuration file is reloaded. Of course, if it's the server
> startup time, such invalid setting should prevent the server from starting up.
>
> Regarding recovery_target_timeline, isn't it better to mark that parameter
> as PGC_POSTMASTER? I'm not sure if we really want to change the recovery
> target timeline without server restart and also not sure if that operation is
> safe.

It's one of the main objectives of the patch to allow user to reset
parameters online so I don't think we should set PGC_POSTMASTER.

Of course, if the user provides an incorrect parameter value for
search, we have no way to understand their intentions. But we do know
they wanted to reset the parameter, so we should not continue using
the old parameter.

I understand your wish to throw a WARNING and "stay up", but in the
context of a recovery the whole purpose of starting the server is to
recover. If we are unable to do that because the user has failed to
set the parameter correctly then we should throw ERROR.

-- 
Simon Riggshttp://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services


-- 
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] Push down more full joins in postgres_fdw

[Ashutosh Bapat]

On Wed, Jan 11, 2017 at 5:45 PM, Etsuro Fujita
 wrote:
> On 2017/01/05 21:38, Ashutosh Bapat wrote:
>>
>> On Thu, Jan 5, 2017 at 5:51 PM, Etsuro Fujita
>>  wrote:
>>>
>>> On 2017/01/05 21:11, Ashutosh Bapat wrote:
>
>
> On 2017/01/03 17:28, Ashutosh Bapat wrote:
>>
>> Also, in this function, if fpinfo->tlist is already set, why do we
>> want
>> to
>> build it again?
>
>
 IIUC, for a relation with use_remote_estimates we will deparse the
 query twice and will build the targetlist twice.
>
>
>>> That's right.  We could avoid the duplicate work the way you proposed,
>>> but I
>>> was thinking to leave that for another patch.  Should we do that in this
>>> patch?
>
>
>> If you are agree that the change is needed, it's better to do it in
>> this patch itself if we can, instead of a one liner patch.
>
>
> While working on this, I noticed a weird case.  Consider:
>
> postgres=# explain verbose select 1 from ft1 left join ft2 on (ft1.a =
> ft2.a) inner join test on (true);
>QUERY PLAN
> -
>  Nested Loop  (cost=100.00..103.06 rows=1 width=4)
>Output: 1
>->  Foreign Scan  (cost=100.00..102.04 rows=1 width=0)
>  Relations: (public.ft1) LEFT JOIN (public.ft2)
>  Remote SQL: SELECT NULL FROM (public.t1 r1 LEFT JOIN public.t2 r2
> ON (((r1.a = r2.a
>->  Seq Scan on public.test  (cost=0.00..1.01 rows=1 width=0)
>  Output: test.a, test.b
> (7 rows)
>
> In this case the fpinfo->tlist of the foreign join is NIL, so whether or not
> the tlist is already built cannot be discriminated by the fpinfo->tlist.  We
> might need another flag to show that the tlist has been built already.
> Since this is an existing issue and we would need to give careful thought to
> this, so I'd like to leave this for another patch.

I think in that case, relation's targetlist will also be NIL or
contain no Var node. It wouldn't be expensive to build it again and
again. That's better than maintaining a new flag. This is just a
suggestion, for an additional check, you might want to check
rel->reltarget->exprs for NIL. But I think we don't need it.

-- 
Best Wishes,
Ashutosh Bapat
EnterpriseDB Corporation
The Postgres Database Company


-- 
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers