Re: Pluggable Storage - Andres's take

[Haribabu Kommi]

On Mon, Oct 29, 2018 at 7:40 AM Dmitry Dolgov <9erthali...@gmail.com> wrote:

> > On Fri, 26 Oct 2018 at 13:25, Haribabu Kommi 
> wrote:
> >
> > Here I attached the cumulative patch with all fixes that are shared in
> earlier mails by me.
> > Except fast_default test, rest of test failures are fixed.
>
> Hi,
>
> If I understand correctly, these patches are for the branch
> "pluggable-storage"
> in [1] (at least I couldn't apply them cleanly to "pluggable-zheap"
> branch),
> right?


Yes, the patches attached are for pluggable-storage branch.


> I've tried to experiment a bit with the current status of the patch, and
> accidentally stumbled upon what seems to be an issue - when I run pgbench
> agains it with some significant number of clients and script [2]:
>
> $ pgbench -T 60 -c 128 -j 64 -f zipfian.sql
>

Thanks for testing the patches.


> I've got for some client an error:
>
> client 117 aborted in command 5 (SQL) of script 0; ERROR:
> unrecognized heap_update status: 1
>

This error is for the tuple state of HeapTupleInvisible, As per the comments
in heap_lock_tuple, this is possible in ON CONFLICT UPDATE, but because
of reorganizing of the table_lock_tuple out of EvalPlanQual(), the invisible
error is returned in other cases also. This case is missed in the new code.


> This problem couldn't be reproduced on the master branch, so I've tried to
> investigate it. It comes from nodeModifyTable.c:1267, when we've got
> HeapTupleInvisible as a result, and this value in turn comes from
> table_lock_tuple. Everything points to the new way of handling
> HeapTupleUpdated
> result from heap_update, when table_lock_tuple call was introduced. Since I
> don't see anything similar in the master branch, can anyone clarify why is
> this
> lock necessary here?


In the master branch code also, there is a tuple lock that is happening in
EvalPlanQual() function, but pluggable-storage code, the lock is kept
outside
and also function call rearrangements, to make it easier for the table
access
methods to provide their own MVCC implementation.


> Out of curiosity I've rearranged the code, that handles
> HeapTupleUpdated, back to switch and removed table_lock_tuple (see the
> attached
> patch, it can be applied on top of the lastest two patches posted by
> Haribabu)
> and it seems to solve the issue.
>

Thanks for the patch. I didn't reproduce the problem, based on the error
from
your mail, the attached draft patch of handling of invisible tuples in
update and
delete cases should also fix it.

Regards,
Haribabu Kommi
Fujitsu Australia


0001-Handling-HeapTupleInvisible-case.patch
Description: Binary data

RE: COPY FROM WHEN condition

[myungkyu.lim]

Hello,

Basically, this patch worked very well in my tests.

> 3) For COPY TO, the WHEN clause is accepted but ignored, leading to confusing 
> cases like this:

I found same issue.

postgres=# copy t1 to '/home/lmk/t1.data' when c1 < 1;

In the 'COPY TO' statement, 'WHEN clause' does not do any extra work.
(include error or hint)

> 4) There are some minor code style issues in copy.c - the variable is 
> misnamed as when_cluase, there are no spaces after 'if' etc. See the attached 
> patch fixing this.

It is recommended to use 'pg tool' when you finish development.

src/tools/pgindent/pgindent

pgindent is used to fix the source code style to conform to pg standards.

Best regards,
Myungkyu, Lim

why commutator doesn't work?

[Pavel Stehule]

Hi

I try to create operator + for varchar and integer with Oracle behave.

create or replace function sum(varchar, int)
returns int as $$ select $1::int + $2 $$ language sql;

create operator + (function = sum, leftarg = varchar, rightarg = int,
commutator = +);

create table foo2(a varchar);
insert into foo2 values('10');
select a + 1 from foo2; -- it is ok

but

select 1 + a from foo2; -- fails

ERROR:  operator is only a shell: integer + character varying
LINE 1: select 1 + a  from foo2;

Why? This should be solved by COMMUTATOR = +

Regards

Pavel

Re: partition tree inspection functions

[Michael Paquier]

On Fri, Oct 19, 2018 at 06:55:09PM +0900, Amit Langote wrote:
> Yeah, we could make it the responsibility of the callers of
> find_all_inheritors and find_inheritance_children to check relhassubclass
> as an optimization and remove any reference to relhassubclass from
> pg_inherits.c.  Although we can write such a patch, it seems like it'd be
> bigger than the patch to ensure the correct value of relhassubclass for
> indexes, which I just posted on the other thread [1].

And what is present as patch 0001 on this thread has been committed as
55853d6, so we are good for this part.

>> Anyway, it seems that you are right here.  Just setting relhassubclass
>> for partitioned indexes feels more natural with what's on HEAD now.
>> Even if I'd like to see all those hypothetical columns in pg_class go
>> away, that cannot happen without a close lookup at the performance
>> impact.
> 
> Okay, I updated the patch on this thread.

Thanks for the new version.

> Since the updated patch depends on the correct value of relhassubclass
> being set for indexes, this patch should be applied on top of the other
> patch.  I've attached here both.

-   if (!has_subclass(parentrelId))
+   if (get_rel_relkind(parentrelId) != RELKIND_PARTITIONED_INDEX &&
+   !has_subclass(parentrelId))
return NIL;

You don't need this bit anymore, relhassubclass is now set for
partitioned indexes.

+  ereport(ERROR,
+  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
+   errmsg("\"%s\" is not a table, a foreign table, or an index",
+  get_rel_name(rootrelid;
Should this also list "partitioned tables and partitioned indexes"?  The
style is heavy, but that maps with what pgstattuple does..

The tests should include also something for a leaf index when fed to
pg_partition_tree() (in order to control the index names you could just
attach an index to a partition after creating it, but I leave that up to
you).

+ 
pg_partition_tree(oid)
+   setof record

The change to regclass has not been reflected yet in the documentation
and the implementation, because...

> Another change I made is something Robert and Alvaro seem to agree about
> -- to use regclass instead of oid type as input/output columns.

...  I am in minority here, it feels lonely ;)
--
Michael


signature.asc
Description: PGP signature

Re: [PATCH] Tab complete EXECUTE FUNCTION for CREATE (EVENT) TRIGGER

[Michael Paquier]

On Fri, Oct 26, 2018 at 05:11:59PM +0100, Dagfinn Ilmari Mannsåker wrote:
> Thanks!  I've updated the commitfest entry with you as the committer and
> Tom as the reviewer, and marked it as committed.

Thanks a lot for updating the CF app.  I have been a bit sloppy here.
--
Michael


signature.asc
Description: PGP signature

RE: [Proposal] Add accumulated statistics for wait event

[Yotsunaga, Naoki]

On Thu, Oct 4, 2018 at 8:22 PM, Yotsunaga Naoki wrote:

Hi, I understood and thought of your statistic comment once again. In the case 
of sampling, is there enough statistic to investigate?
In the case of a long SQL, I think that it is possible to obtain a sufficient 
sampling number.

However, in the case of about 1 minute of SQL, only 60 samples can be obtained 
at most.
#Because legard’s comment.
https://www.postgresql.org/message-id/1539158356795-0.post%40n3.nabble.com

Does this sampling number of 60 give information that I really want?
Perhaps it is not to miss the real problem part?
---
Naoki, Yotsunaga.

Re: Ordered Partitioned Table Scans

[David Rowley]

Thanks for looking at this.

On 28 October 2018 at 03:49, Julien Rouhaud  wrote:
> I just had a look at your patch.  I see that you implemented only a
> subset of the possible optimizations (only the case for range
> partitionoing without subpartitions).  This has been previously
> discussed, but we should be able to do similar optimization for list
> partitioning if there's no interleaved values, and also for some cases
> of multi-level partitioning.

I had thought about these cases but originally had thought they would
be more complex to implement than I could justify. On review, I've
found some pretty cheap ways to handle both sub-partitions and for
LIST partitioned tables.  Currently, with LIST partitioned tables I've
coded it to only allow the optimisation if there's no DEFAULT
partition and all partitions are defined with exactly 1 Datum. This
guarantees that there are no interleaved values, but it'll just fail
to optimise cases like FOR VALUES IN(1,2) + FOR VALUES In(3,4).   The
reason that I didn't go to the trouble of the additional checks was
that I don't really want to add any per-partition overhead to this.
If RelOptInfo had a Bitmapset of live partitions then we could just
check the partitions that survived pruning.  Amit Langote has a
pending patch which does that and some other useful stuff, so maybe we
can delay fixing that until the dust settles a bit in that area. Amit
and I are both working hard to remove all these per-partition
overheads. I imagine he'd also not be in favour of adding code that
does something for all partitions when we've pruned down to just 1.
I've personally no objection to doing the required additional
processing for the non-pruned partitions only. We could also then fix
the case where we disable the optimisation if there's a DEFAULT
partition without any regards to if it's been pruned or not.

> Concerning the implementation, there's at least one issue: it assumes
> that each subpath of a range-partitioned table will be ordered, with
> is not guaranteed.  You need to to generate explicit Sort nodes nodes
> (in the same order as the query_pathkey) for partitions that don't
> have an ordered path and make sure that this path is used in the
> Append.  Here's a simplistic case showing the issue (sorry, the
> partition names are poorly chosen):

Thanks for noticing this. I had been thrown off due to the fact that
Paths are never actually created for these sorts. On looking further I
see that we do checks during createplan to see if the path is
suitability sorted and just create a sort node if it's not. This seems
to go against the whole point of paths, but I'm not going to fight for
changing it, so I've just done the Append the same way as MergeAppend
handles it.

> Also, if a LIMIT is specified, it should be able to give better
> estimates, at least if there's no qual.  For instance:
>
> EXPLAIN SELECT * FROM simple ORDER BY id LIMIT 10;
>QUERY PLAN
> >
> --->
>  Limit  (cost=0.00..0.35 rows=10 width=15)
>->  Append  (cost=0.00..7705.56 rows=21 width=15)
>  ->  Seq Scan on simple_0_1  (cost=0.00..309.00 rows=2 width=15)
>  ->  Index Scan using simple_1_2_id_idx on simple_1_2
> (cost=0.29..3148.28 rows=9 width=14)
>  ->  Index Scan using simple_2_3_id_idx on simple_2_3
> (cost=0.29..3148.29 rows=10 width=16)
> (5 rows)
>
> In this case, we should estimate that the SeqScan (or in a corrected
> version the Sort) node should not return more than 10 rows, and each
> following partition should be scanned at all, and cost each path
> accordingly.  I think that this is quite important, for instance to
> make sure that natively sorted Append is chosen over a MergeAppend
> when there are some subpath with explicit sorts, because with the
> Append we probably won't have to execute all the sorts if the previous
> partition scans returned enough rows.

In my patch, I'm not adding any additional paths. I'm just adding an
Append instead of a MergeAppend.  For what you're talking about the
limit only needs to be passed into any underlying Sort so that it can
become a top-N sort.  This is handled already in create_limit_path().
Notice in the plan you pasted above that the limit has a lower total
cost than its Append subnode. That's because create_limit_path()
weighted the Limit total cost based on the row count of the limit and
its subpath. ... 7705.56 / 21 * 10 = ~0.35.

> FWIW, both those cases were handled (probably with some bugs though)
> in the previous patches Ronan and I sent some time ago.  Also, I did
> not forget about this feature, I planned to work on it in hope to have
> it included in pg12.  However, I won't have a lot of time to work on
> it before December.

I apologise for not noticing your patch. I only went as far as
checking the November 

Re: Multiple Wait Events for extensions

[Michael Paquier]

On Sun, Oct 28, 2018 at 10:12:48AM -0700, legrand legrand wrote:
> An other idea that may be called a "better wait event error handling"
> would have be to display:
> 
> "???-xx" unknown event type (xx being the associated number) 
> in pgstat_get_wait_event_type()
> 
> "unknown wait event - yy" unknown event name (yy being the associated
> number)
> in pgstat_get_wait_event()
> 
> giving an extended range of spare events ;o)

Those look like half-baked workarounds in my opinion.  What we may want
to finish with is the possibility to register a new event with a custom
string bundled so as an extension loaded via shared_preload_libraries
could call it.  That's not a small amount of work.
--
Michael


signature.asc
Description: PGP signature

Re: Conflicting option checking in pg_restore

[Michael Paquier]

On Sun, Oct 28, 2018 at 10:02:02PM +0100, Daniel Gustafsson wrote:
>> On 28 Oct 2018, at 19:42, Fabien COELHO  wrote:
 Function RestoreArchive is called both from pg_dump & pg_restore, so now
 the sanity check is not performed for the former (which does not have the
 -1 option, though). Moreover, the function is noted "Public", which may
 suggest that external tools could take advantage of it, and if so it
 suggests that maybe it is not wise to remove the test. Any opinion around?

I would still need to see such a tool, and they would need most of the
stuff that pg_dump has already to get the object data.  As far as I know
it is marked as public because plain-text mode of pg_dump uses it.

>>> Wouldn't ropt->single_txn be undefined when called from pg_dump ?
>> 
>> Yes, probably.
> 
> pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
> allocates it with pg_malloc0(), making single_txn false.

Which is why it would not be a problem for pg_dump.  The cross-option
check you are moving has been added as of 3a819b07, still you are right
that such checks should happen at the beginning so the suggestion of
this thread seems right to me.

Any objections from others?
--
Michael


signature.asc
Description: PGP signature

Re: jsonpath

[Tomas Vondra]

Hi,

On 10/02/2018 04:33 AM, Michael Paquier wrote:
> On Sat, Sep 08, 2018 at 02:21:27AM +0300, Nikita Glukhov wrote:
>> Attached 18th version of the patches rebased onto the current master.
> 
> Nikita, this version fails to apply, as 0004 has conflicts with some
> regression tests.  Could you rebase?  I am moving the patch to CF
> 2018-11, waiting for your input.
> --
> Michael
> 

As Michael mentioned, the patch does not apply anymore, so it would be
good to provide a rebased version for CF 2018-11. I've managed to do
that, as the issues are due to minor bitrot, so that I can do some quick
review of the current version.

I haven't done much testing, pretty much just compiling, running the
usual regression tests and reading through the patches. I plan to do
more testing once the rebased patch is submitted.

Now, a couple of comments based on eye-balling the diffs.


1) There are no docs, which makes it pretty much non-committable for
now. I know there is [1] and it was a good intro for the review, but we
need something as a part of our sgml docs.


2) 0001 says this:

*typmod = -1; /* TODO implement FF1, ..., FF9 */

but I have no idea what FF1 or FF9 are. I guess it needs to be
implemented, or explained better.


3) The makefile rule for scan.o does this:

+# Latest flex causes warnings in this file.
+ifeq ($(GCC),yes)
+scan.o: CFLAGS += -Wno-error
+endif

That seems a bit ugly, and we should probably try to make it work with
the latest flex, instead of hiding the warnings. I don't think we have
any such ad-hoc rules in other Makefiles. If we really need it, can't we
make it part of configure, and/or restrict it depending on flex version?


4) There probably should be .gitignore rule for jsonpath_gram.h, just
like for other generated header files.


5) jbvType says jbvDatetime is "virtual type" but does not explain what
it is. IMHO that deserves a comment or something.


6) I see the JsonPath definition says this about header:

/* just version, other bits are reservedfor future use */

but the very next thing it does is defining two pieces stored in the
header - version AND "lax" mode flag. Which makes the comment invalid
(also, note the missing space after "reserved").

FWIW, I'd use JSONPATH_STRICT instead of JSONPATH_LAX. The rest of the
codebase works with "strict" flags passed around, and it's easy to
forget to negate the flag somewhere (at least that's my experience).


7) I see src/include/utils/jsonpath_json.h adds support for plain json
by undefining various jsonb macros and redirecting them to the json
variants. I find that rather suspicious - imagine you're investigating
something in code using those jsonb macros, and wondering why it ends up
calling the json stuff. I'd be pretty confused ...

Also, some of the redefinitions are not really needed for example
JsonbWrapItemInArray and JsonbWrapItemsInArray are not used anywhere
(and neither are the json variants).


8) I see 0002 defines IsAJsonbScalar like this:

  #define IsAJsonbScalar(jsonbval)  (((jsonbval)->type >= jbvNull && \
 (jsonbval)->type <= jbvBool) || \
 (jsonbval)->type == jbvDatetime)

while 0004 does this

  #define jspIsScalar(type) ((type) >= jpiNull && (type) <= jpiBool)

I know those are for different data types (jsonb vs. jsonpath), but I
suppose jspIsScalar should include the datetime too.

FWIW JsonPathItemType would deserve better documentation what the
various items mean (a comment for each line would be enough). I've been
wondering if "jpiDouble" means scalar double value until I realized
there's a ".double()" function for paths.


9) It's generally a good idea to make the individual pieces committable
separately, but that means e.g. the regression tests have to pass after
each patch. At the moment that does not seem to be the case for 0002,
see the attached file. I'm running with -DRANDOMIZE_ALLOCATED_MEMORY,
not sure if that's related.

regards


[1] https://github.com/obartunov/sqljsondoc/blob/master/README.jsonpath.md

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

*** /home/user/work/postgres/src/test/regress/expected/json_jsonpath.out
2018-10-28 22:20:53.788520950 +0100
--- /home/user/work/postgres/src/test/regress/results/json_jsonpath.out 
2018-10-28 22:22:37.786520950 +0100
***
*** 281,291 
  (1 row)
  
  select json '{}' @* 'lax $[0]';
!  ?column? 
! --
!  {}
! (1 row)
! 
  select json '[1]' @* 'lax $[0]';
   ?column? 
  --
--- 281,287 
  (1 row)
  
  select json '{}' @* 'lax $[0]';
! ERROR:  unknown jsonb value type: 21030544
  select json '[1]' @* 'lax $[0]';
   ?column? 
  --
***
*** 320,330 
  (1 row)
  
  select json '{}' @* 'lax $[last]';
!  ?column? 
! --
!  {}
! (1 row)
! 
  select json '[1,2,3]' @* '$[last]';
   ?column? 
  --
--- 316,322 
  (1 row)

Re: SQL:2011 PERIODS vs Postgres Ranges?

[Paul Jungwirth]

Hi Jeff,

Thanks for sharing your thoughts and encouragement! :-)

> The model in [7] is
> based heavily on pack/unpack operators, and it's hard for me to see
> how those fit into SQL. Also, the pack/unpack operators have some
> theoretical weirdness that the book does not make clear*.
>
> *: My question was about the significance
> of the order when packing on two intervals. Hugh Darwen was kind
> enough to reply at length, and offered a lot of insight, but was still
> somewhat inconclusive.

I'd be interested in seeing that conversation if you ever find it again.

I really like how Date/Darwen/Lorentzos use pack/unpack to explain 
temporal operations as operating on every concurrent "instant" 
separately, and then bringing the adjacent instants back together into 
ranges again. Even if you don't materialize that approach, conceptually 
it makes it easy to understand what's going on.


So what is great about the patch from Anton Dignös 
(https://www.postgresql-archive.org/PROPOSAL-Temporal-query-processing-with-range-types-tt5913058.html) 
is that (like Date/Darwen/Lorentzos) you still have temporal variants 
for every operator in the relational algebra, but they give 
straightforward & efficient implementations of each based on traditional 
operators plus just their two new "normalize" and "align" operations. (I 
think they renamed these in later papers/patches though?) Their main 
paper is at https://files.ifi.uzh.ch/boehlen/Papers/modf174-dignoes.pdf 
if anyone wants to read it. It's short! :-)


The biggest challenge implementing temporal operators in plain SQL is 
merging/splitting ranges from the left & right sides of an operator so 
they line up. A single row can get split into multiple rows, or several 
rows might be merged into one, etc. You can see how tricky Snodgrass's 
"coalesce" operation is in his book. I gave some example SQL to 
implement coalesce with UNNEST plus a range_agg function at 
https://github.com/pjungwir/range_agg but with the Dignös approach I 
don't think you'd need that. Normalize/align targets roughly the same 
problem.


Anyway I'd be curious whether the theoretical weirdness you found in 
pack/unpack also applies to normalize/align.


Yours,

--
Paul  ~{:-)
p...@illuminatedcomputing.com

Re: Conflicting option checking in pg_restore

[Daniel Gustafsson]

> On 28 Oct 2018, at 19:42, Fabien COELHO  wrote:

>>> Function RestoreArchive is called both from pg_dump & pg_restore, so now
>>> the sanity check is not performed for the former (which does not have the
>>> -1 option, though). Moreover, the function is noted "Public", which may
>>> suggest that external tools could take advantage of it, and if so it
>>> suggests that maybe it is not wise to remove the test. Any opinion around?
>> 
>> [...]
>> 
>> Wouldn't ropt->single_txn be undefined when called from pg_dump ?
> 
> Yes, probably.

pg_dump creates the RestoreOptions struct with NewRestoreOptions() which
allocates it with pg_malloc0(), making single_txn false.

cheers ./daniel

Re: Conflicting option checking in pg_restore

[Daniel Gustafsson]

> Could you add the patch to the CF app?
> 
> https://commitfest.postgresql.org/20/

Done.

>> Checking for conflicting options in pg_restore was mostly done in main() with
>> one check deferred until RestoreArchive().  Reading the git history makes it
>> seem like it simply happened, without the disjoint checking being 
>> intentional.
>> Am I reading it right that we can consolidate all the option checking to
>> main()?  The attached patch does that, and also rewords the error message to
>> make it similar to the other option checks.
> 
> Patch applies cleanly, compiles, both global and local "make check" ok.

Thanks for reviewing!

> As there are no test changes, this is not tested. I'd suggest to add it to 
> "src/bin/pg_dump/t/001_basic.pl”.

Excellent point, I’ve added a test in the attached revision.

> There is a possible catch:
> 
> Function RestoreArchive is called both from pg_dump & pg_restore, so now the 
> sanity check is not performed for the former (which does not have the -1 
> option, though). Moreover, the function is noted "Public", which may suggest 
> that external tools could take advantage of it, and if so it suggests that 
> maybe it is not wise to remove the test. Any opinion around?
> 
> Maybe the check could be kept in RestoreArchive with a comment to say it is 
> redundant, but the check could be performed in pg_restore as well for the 
> sake of having an better and more homogeneous error message.

Perhaps, we don’t really test for all other potential misconfigurations of the
options so I can go either way.  It’s also a cheap enough operation.  Do you
think it should be a check like today or an assertion?

cheers ./daniel



pg_restore_options-v2.patch
Description: Binary data

Re: Conflicting option checking in pg_restore

[Fabien COELHO]

Hello Narayanan,


There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now
the sanity check is not performed for the former (which does not have the
-1 option, though). Moreover, the function is noted "Public", which may
suggest that external tools could take advantage of it, and if so it
suggests that maybe it is not wise to remove the test. Any opinion around?


[...]

Wouldn't ropt->single_txn be undefined when called from pg_dump ?


Yes, probably.

Unless I missed something here, I think it is logical to just move the 
relevant code to pg_restore main.


My point is that given the "Public" comment and that some care is taken to 
put everything in a special struct, I was wondering whether external tools 
may use this function, in which case the check would be left out.


--
Fabien.

Re: Pluggable Storage - Andres's take

[Dmitry Dolgov]

> On Fri, 26 Oct 2018 at 13:25, Haribabu Kommi  wrote:
>
> Here I attached the cumulative patch with all fixes that are shared in 
> earlier mails by me.
> Except fast_default test, rest of test failures are fixed.

Hi,

If I understand correctly, these patches are for the branch "pluggable-storage"
in [1] (at least I couldn't apply them cleanly to "pluggable-zheap" branch),
right? I've tried to experiment a bit with the current status of the patch, and
accidentally stumbled upon what seems to be an issue - when I run pgbench
agains it with some significant number of clients and script [2]:

$ pgbench -T 60 -c 128 -j 64 -f zipfian.sql

I've got for some client an error:

client 117 aborted in command 5 (SQL) of script 0; ERROR:
unrecognized heap_update status: 1

This problem couldn't be reproduced on the master branch, so I've tried to
investigate it. It comes from nodeModifyTable.c:1267, when we've got
HeapTupleInvisible as a result, and this value in turn comes from
table_lock_tuple. Everything points to the new way of handling HeapTupleUpdated
result from heap_update, when table_lock_tuple call was introduced. Since I
don't see anything similar in the master branch, can anyone clarify why is this
lock necessary here? Out of curiosity I've rearranged the code, that handles
HeapTupleUpdated, back to switch and removed table_lock_tuple (see the attached
patch, it can be applied on top of the lastest two patches posted by Haribabu)
and it seems to solve the issue.

[1]: https://github.com/anarazel/postgres-pluggable-storage
[2]: https://gist.github.com/erthalion/c85ba0e12146596d24c572234501e756


unrecognized_heap_status.patch
Description: Binary data

Re: SQL:2011 PERIODS vs Postgres Ranges?

[Jeff Davis]

On Sun, 2018-10-21 at 22:10 +0300, Heikki Linnakangas wrote:
> On 21/10/2018 21:17, Paul A Jungwirth wrote:
> > 3. Build our own abstractions on top of ranges, and then use those
> > to
> > implement PERIOD-based features. This is the least clear option,
> > and I
> > imagine it would require a lot more design effort. Our range types
> > are
> > already a step in this direction. Does anyone think this approach
> > has
> > promise? If so I can start thinking about how we'd do it. I imagine
> > we
> > could use a lot of the ideas in [7].
> > ...
> > [7] C. J. Date, Hugh Darwen, Nikos Lorentzos. Time and Relational
> > Theory, Second Edition: Temporal Databases in the Relational Model
> > and
> > SQL. 2nd edition, 2014.
> 
> +1 on this approach. I think [7] got the model right. If we can 
> implement SQL-standard PERIODs on top of it, then that's a bonus,
> but 
> having sane, flexible, coherent set of range operators is more
> important 
> to me.

+1 for approach #3 from me as well. It was my original intention for
range types, though my first priority was utility and not the standard.
I think we are likely to run into a few areas where they aren't a
perfect fit to the standard, but I think it's a promising approach and
we can probably work around those issues by using special operators.

> What are we missing? It's been years since I read that book, but
> IIRC 
> temporal joins is one thing, at least. What features do you have in
> mind?

We do support temporal joins, just not as efficiently as I'd like, and
the language doesn't make it quite as clear as it could be.

I look at that book as a source of inspiration, but I don't think it's
simple to map features one-to-one. For instance, the model in [7] is
based heavily on pack/unpack operators, and it's hard for me to see how
those fit into SQL. Also, the pack/unpack operators have some
theoretical weirdness that the book does not make clear*.

Regards,
Jeff Davis

*: I asked in a temporal discussion group (that was unfortunately a
part of LinkedIn circa 2011 and I can't find any reference to the
discussion outside my mailbox). My question was about the significance
of the order when packing on two intervals. Hugh Darwen was kind enough
to reply at length, and offered a lot of insight, but was still
somewhat inconclusive.

Re: INSTALL file

[Andrew Dunstan]

On 10/28/2018 08:13 AM, Andreas 'ads' Scherbaum wrote:


Hello,

while working with Google Code-In students, there is one task: "clone 
PostgreSQL from git repository, and build from source".


This brought up an interesting problem: the README refers to an 
"INSTALL" file, which is present in packages, but not in the source 
repo. This is very confusing for beginners, and should be avoided.


There are a couple options to fix this:

 1. Update the README, and remove the "INSTALL" reference
 2. Create a permanent INSTALL file, and possibly overwrite it during
packaging
 3. Add different INSTALL files, based on the platform (Linux, BSD,
Windows, Source)


Regards,






See README.git

cheers

andrew

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

Re: [HACKERS] [PATCH] Incremental sort

[Tomas Vondra]

Hi Alexander,

On 06/01/2018 04:22 PM, Alexander Korotkov wrote:
> Hi, James!
> 
> On Thu, May 31, 2018 at 11:10 PM James Coleman  > wrote:
> 
> I've attached an updated copy of the patch that applies cleanly to
> current master.
> 
> 
> Thank you for rebasing this patch.  Next time sending a patch,
> please make sure you've bumped its version, if even you made no
> changes there besides to pure rebase. Otherwise, it would be hard to
> distinguish patch versions, because patch files have exactly same
> names.
> 
> I'd like to note, that I'm going to provide revised version of this 
> patch to the next commitfest. After some conversations at PGCon
> regarding this patch, I got more confident that providing separate
> paths for incremental sorts is right.  In the next revision of this
> patch, incremental sort paths would be provided in more cases.
> 

Do you plan to submit an updated patch version for the November CF? It
would be good to look at the costing model soon, otherwise it might end
up missing PG12, and that would be unfortunate.

regards

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

Re: FETCH FIRST clause WITH TIES option

[Tomas Vondra]

Hello Surafel,

On 10/26/2018 12:28 PM, Surafel Temesgen wrote:
> hello ,
> 
> The WITH TIES keyword is sql standard that specifies any peers of 
> retained rows to retained in the result set too .which means 
> according to ordering key the result set can includes additional rows
> which have ties on the last position, if there are any and It work
> with ORDER BY query.
>

Thanks for the patch. I've looked at it today, and it seems mostly OK,
with a couple of minor issues. Most of it is code formatting and comment
wording issues, so I'm not going to go through them here - see the
attached 0002 patch (0001 is your patch, rebased to current master).

There's one place that I think is incorrect, and that's this bit from
ExecLimit:

}else
/*
 * Get next tuple from subplan, if any.
 */
slot = ExecProcNode(outerPlan);
if (TupIsNull(slot))
{
node->lstate = LIMIT_SUBPLANEOF;
return NULL;
}
if (node->limitOption == WITH_TIES)
{
ExecCopySlot(node->last_slot, slot);
}
node->subSlot = slot;
node->position++;

Note that the "else" guards only the very first line, not the whole
block. This seems wrong, i.e. there should be {} around the whole block.

I'm also suggesting to slightly change the create_limit_plan(), to keep
a single make_limit call. IMHO that makes it easier to understand,
although I admit it's fairly subjective.

One question is whether to make this work for LIMIT too, not just for
FETCH FIRST. I'm not sure how difficult would that be (it should be a
grammar-only tweak I guess), or if it's a good idea in general. But I
find it quite confusing that various comments say things like this:

  LimitOption  limitOption; /* LIMIT with ties or  exact number */

while in fact it does not work with LIMIT.

> 
> The attach patch is a finished implementation of it except it did not
> have a regression test because am not sure of changing the test data set
> for it and I can’t find fetch first clause regression test too.
> 

Well, that's not a very good reason not to have tests for this new
improvement. FWIW there are a couple of places in regression tests where
FETCH FIRST is used, see this:

  $ grep -i 'fetch first' src/test/regress/sql/*
  src/test/regress/sql/hs_standby_allowed.sql:fetch first from hsc;
  src/test/regress/sql/tablesample.sql:FETCH FIRST FROM tablesample_cur;
  src/test/regress/sql/tablesample.sql:FETCH FIRST FROM tablesample_cur;
  src/test/regress/sql/tidscan.sql:FETCH FIRST FROM c;

But those places don't seem like very good match for testing the new
stuff, because those are primarily testing something else. I suggest we
add the new tests into limit.sql.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From 7f5549614c423a1da060da5f7598e9d0836d03a8 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 28 Oct 2018 18:38:47 +0100
Subject: [PATCH 2/2] review

---
 doc/src/sgml/ref/select.sgml|  6 ++--
 src/backend/executor/nodeLimit.c| 52 +++--
 src/backend/optimizer/plan/createplan.c | 25 +++-
 src/include/nodes/execnodes.h   |  4 +--
 src/include/nodes/parsenodes.h  |  4 +--
 src/include/nodes/plannodes.h   |  2 +-
 src/include/nodes/relation.h|  2 +-
 7 files changed, 49 insertions(+), 46 deletions(-)

diff --git a/doc/src/sgml/ref/select.sgml b/doc/src/sgml/ref/select.sgml
index c81ac04108..b649b1ca7a 100644
--- a/doc/src/sgml/ref/select.sgml
+++ b/doc/src/sgml/ref/select.sgml
@@ -1408,9 +1408,9 @@ FETCH { FIRST | NEXT } [ count ] {
 If count is
 omitted in a FETCH clause, it defaults to 1.
 ROW
-WITH TIES option is Used to return two or more rows 
-that tie for last place in the limit results set according to ORDER BY 
-clause (ORDER BY clause must be specified in this case). 
+WITH TIES option is used to return two or more rows
+that tie for last place in the limit results set according to ORDER BY
+clause (ORDER BY clause must be specified in this case).
 and ROWS as well as FIRST
 and NEXT are noise words that don't influence
 the effects of these clauses.
diff --git a/src/backend/executor/nodeLimit.c b/src/backend/executor/nodeLimit.c
index 93f8813972..724a8e09c1 100644
--- a/src/backend/executor/nodeLimit.c
+++ b/src/backend/executor/nodeLimit.c
@@ -132,7 +132,8 @@ ExecLimit(PlanState *pstate)
  * the state machine state to record having done so.
  */
 if (!node->noCount &&
-	node->position - node->offset >= node->count && node->limitOption == WITH_ONLY)
+	node->position - node->offset >= node->count &&
+	node->limitOption == WITH_ONLY)
 {
 	node->lstate = LIMIT_WINDOWEND;
 
@@ -145,23 +146,24 @@ ExecLimit(PlanState *pstate)
 
 	return NULL;
 }
-
 else if (!node->noCount &&
-	node->position - node->offset >= node->count && 

Re: GiST VACUUM

[Andrey Borodin]

Hi everyone!

> 2 окт. 2018 г., в 6:14, Michael Paquier  написал(а):
> Andrey, your latest patch does not apply.  I am moving this to the next
> CF, waiting for your input.

I'm doing preps for CF.
Here's rebased version.

Best regards, Andrey Borodin.


0002-Delete-pages-during-GiST-VACUUM-v17.patch
Description: Binary data


0001-Physical-GiST-scan-in-VACUUM-v17.patch
Description: Binary data
 

Re: Multiple Wait Events for extensions

[legrand legrand]

Michael Paquier-2 wrote
> On Wed, Oct 24, 2018 at 11:18:13AM -0700, legrand legrand wrote:
>> Would a hard coded solution as described here after possible for
>> mid-term?
> 
> I don't think I would commit that as we would want a better solution
> with custom names, but patching Postgres to do so with a custom build
> would be easy enough..

I can't deploy a custom build in my context, and will reuse existing spare
events
like 
- 0x050EU "Activity"-"unknown wait event"
- 0x0B01U "???"-"unknown wait event"
- ...

An other idea that may be called a "better wait event error handling"
would have be to display:

"???-xx" unknown event type (xx being the associated number) 
in pgstat_get_wait_event_type()

"unknown wait event - yy" unknown event name (yy being the associated
number)
in pgstat_get_wait_event()

giving an extended range of spare events ;o)

Regards
PAscal



--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: Multiple Wait Events for extensions

[legrand legrand]

Michael Paquier-2 wrote
> On Wed, Oct 24, 2018 at 11:18:13AM -0700, legrand legrand wrote:
>> Would a hard coded solution as described here after possible for
>> mid-term?
> 
> I don't think I would commit that as we would want a better solution
> with custom names, but patching Postgres to do so with a custom build
> would be easy enough..





--
Sent from: http://www.postgresql-archive.org/PostgreSQL-hackers-f1928748.html

Re: COPY FROM WHEN condition

[Tomas Vondra]

Hi,

I've taken a quick look at this on the way back from pgconf.eu, and it
seems like a nice COPY improvement in a fairly good shape.

Firstly, I think it's a valuable because it allows efficiently importing
a subset of data. Currently, we either have to create an intermediate
table, copy all the data into that, do the filtering, and then insert
the subset into the actual target table. Another common approach that I
see in practice is using file_fdw to do this, but it's not particularly
straight-forward (having to create the FDW servers etc. first) not
efficient (particularly for large amounts of data). This feature allows
skipping this extra step (at least in simpler ETL cases).

So, I like the idea and I think it makes sense.


A couple of comments regarding the code/docs:


1) I think this deserves at least some regression tests. Plenty of tests
already use COPY, but there's no coverage for the new piece. So let's
add a new test suite, or maybe add a couple of tests into copy2.sql.


2) In copy.sqml, the new item is defined like this

WHEN Clause

I suggest we use just WHEN, that's what
the other items do (see ENCODING).

The other thing is that this does not say what expressions are allowed
in the WHEN clause. It seems pretty close to WHEN clause for triggers,
which e.g. mentions that subselects are not allowed. I'm pretty sure
that's true here too, because it fails like this (118 = T_SubLink):

test=# copy t(a,b,c) from '/tmp/t.data' when ((select 1) < 10);
ERROR:  unrecognized node type: 118

So, the patch needs to detect this, produce a reasonable error message
and document the limitations in copy.sqml, just like we do for CREATE
TRIGGER.


3) For COPY TO, the WHEN clause is accepted but ignored, leading to
confusing cases like this:

test=# copy t(a,b,c) to '/tmp/t.data' when ((select 100) < 10);
COPY 151690

So, it contains subselect, but unlike COPY FROM it does not fail
(because we never execute it). The fun part is that the expression is
logically false, so a user might expect it to filter rows, yet we copy
everything.

IMHO we need to either error-out in these cases, complaining about WHEN
not being supported for COPY TO, or make it work (effectively treating
it as a simpler alternative to COPY (subselect) TO).


4) There are some minor code style issues in copy.c - the variable is
misnamed as when_cluase, there are no spaces after 'if' etc. See the
attached patch fixing this.

AFAICS we could just get rid of the extra when_cluase variable and mess
with the cstate->whenClause directly, depending on how (3) gets fixed.


5) As I mentioned, the CREATE TRIGGER already has WHEN clause, but it
requires it to be 'WHEN (expr)'. I suggest we do the same thing here,
requiring the parentheses.


6) The skip logic in CopyFrom() seems to be slightly wrong. It does
work, but the next_record label is defined after CHECK_FOR_INTERRUPTS()
so a COPY will not respond to Ctrl-C unless it finds a row matching the
WHEN condition. If you have a highly selective condition, that's a bit
inconvenient.

It also skips

MemoryContextSwitchTo(GetPerTupleMemoryContext(estate));

so I wonder what the heap_form_tuple() right after the next_record label
will use for tuples right after a skipped one. I'd bet it'll use the
oldcontext (essentially the long-lived context), essentially making it
a memory leak.

So I suggest to get rid of the next_record label, and use 'continue'
instead of the 'goto next_record'.


regards

-- 
Tomas Vondra  http://www.2ndQuadrant.com
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
>From fcf4b43dae7062f4786536fb262a665ad0b05b26 Mon Sep 17 00:00:00 2001
From: Tomas Vondra 
Date: Sun, 28 Oct 2018 16:09:37 +0100
Subject: [PATCH 2/2] review

---
 doc/src/sgml/ref/copy.sgml |  2 +-
 src/backend/commands/copy.c| 22 +++---
 src/backend/parser/gram.y  |  2 +-
 src/include/nodes/parsenodes.h |  2 +-
 4 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 8088e779b6..17bedf95cc 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -366,7 +366,7 @@ COPY { table_name [ ( 
 

-WHEN Clause
+WHEN
 

 The optional WHEN clause has the general form
diff --git a/src/backend/commands/copy.c b/src/backend/commands/copy.c
index 192a1c576b..8e87605135 100644
--- a/src/backend/commands/copy.c
+++ b/src/backend/commands/copy.c
@@ -803,7 +803,7 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 	Relation	rel;
 	Oid			relid;
 	RawStmt*query = NULL;
-	Node*when_cluase= NULL;
+	Node	   *whenClause = NULL;
 
 	/*
 	 * Disallow COPY to/from file or program except to users with the
@@ -857,19 +857,19 @@ DoCopy(ParseState *pstate, const CopyStmt *stmt,
 			NULL, false, false);
 		rte->requiredPerms = (is_from ? ACL_INSERT : ACL_SELECT);
 
-		if(stmt->whenClause)
+		if (stmt->whenClause)
 		{
 			/* add rte 

Re: Conflicting option checking in pg_restore

[Narayanan V]

Hi
On Sun, Oct 28, 2018 at 12:25 PM Fabien COELHO  wrote:

>
> Hallå Daniel,
>
> > Checking for conflicting options in pg_restore was mostly done in main()
> with
> > one check deferred until RestoreArchive().  Reading the git history
> makes it
> > seem like it simply happened, without the disjoint checking being
> intentional.
> > Am I reading it right that we can consolidate all the option checking to
> > main()?  The attached patch does that, and also rewords the error
> message to
> > make it similar to the other option checks.
>
> Patch applies cleanly, compiles, both global and local "make check" ok.
>
> As there are no test changes, this is not tested. I'd suggest to add it to
> "src/bin/pg_dump/t/001_basic.pl".
>
> There is a possible catch:
>
> Function RestoreArchive is called both from pg_dump & pg_restore, so now
> the sanity check is not performed for the former (which does not have the
> -1 option, though). Moreover, the function is noted "Public", which may
> suggest that external tools could take advantage of it, and if so it
> suggests that maybe it is not wise to remove the test. Any opinion around?
>

The RestoreArchive Method extracts the RestoreOptions from the Archive
Handle.
Please see the relevant code below,

,
| void
| RestoreArchive(Archive *AHX)
| {
| ArchiveHandle *AH = (ArchiveHandle *) AHX;
| RestoreOptions *ropt = AH->public.ropt;
`

Wouldn't ropt->single_txn be undefined when called from pg_dump ?

Unless I missed something here, I think it is logical to just move the
relevant code to
pg_restore main.

- Tested the patch on a machine running -Ubuntu 18.04.1 LTS
- Patch applied cleanly.
- The source built post application of the patch.
- make check passed.
,
| ===
|  All 187 tests passed.
| ===
`

+1 from me.

Thank you,
Narayanan



> Maybe the check could be kept in RestoreArchive with a comment to say it
> is redundant, but the check could be performed in pg_restore as well for
> the sake of having an better and more homogeneous error message.
>
> --
> Fabien.

INSTALL file

[Andreas 'ads' Scherbaum]

Hello,

while working with Google Code-In students, there is one task: "clone
PostgreSQL from git repository, and build from source".

This brought up an interesting problem: the README refers to an "INSTALL"
file, which is present in packages, but not in the source repo. This is
very confusing for beginners, and should be avoided.

There are a couple options to fix this:


   1. Update the README, and remove the "INSTALL" reference
   2. Create a permanent INSTALL file, and possibly overwrite it during
   packaging
   3. Add different INSTALL files, based on the platform (Linux, BSD,
   Windows, Source)


Regards,

-- 
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project

Re: CVE-2017-7484-induced bugs, or, btree cmp functions are not leakproof?

[Dilip Kumar]

On Sat, Oct 27, 2018 at 10:07 AM Dilip Kumar  wrote:
>
> On Fri, Oct 26, 2018 at 1:12 PM Amit Langote
>  wrote:
> >
> > On 2018/10/25 19:54, Dilip Kumar wrote:
> > > On Mon, Oct 22, 2018 at 7:47 PM Tom Lane  wrote:
> > >> Amit Langote  writes:
> > >>> But maybe for the case under question, that's irrelevant, because
> > >>> we're only interested in access to inherited columns as those are the
> > >>> only ones that can be accessed in queries via parent.
> > >>
> > >> Yeah, that's what I thought.  It seems like it should be possible to base
> > >> all stats access decisions off the table actually named in the query,
> > >> because only columns appearing in that table could be referenced, and 
> > >> only
> > >> that table's permissions actually get checked at runtime.
> > >>
> > >> I guess it's possible that a child table could have, say, an index on
> > >> column X (inherited) and column Y (local) and that some aspect of costing
> > >> might then be interested in the behavior of column Y, even though the
> > >> query could only mention X not Y.  But then we could fall back to the
> > >> existing behavior.
> > >
> > > Basically, if the relation is RELOPT_OTHER_MEMBER_REL, we can
> > > recursively fetch its parent until we reach to the base relation
> > > (which is actually named in the query). And, once we have the base
> > > relation we can check ACL on that and set vardata->acl_ok accordingly.
> > > Additionally, for getting the parent RTI we need to traverse
> > > "root->append_rel_list". Another alternative could be that we can add
> > > parent_rti member in RelOptInfo structure.
> >
> > Adding parent_rti would be a better idea [1].  I think that traversing
> > append_rel_list every time would be inefficient.
> > [1] I've named it inh_root_parent in one of the patches I'm working on
> > where I needed such a field (https://commitfest.postgresql.org/20/1778/)
> Ok, Make sense. I have written a patch by adding this variable.
> There is still one FIXME in the patch, basically, after getting the
> baserel rte I need to convert child varattno to parent varattno
> because in case of inheritance that can be different.  Do we already
> have any mapping from child attno to parent attno or we have to look
> up the cache.

Attached patch handles this issue.

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


bug_fix_childrel_stat_access_v2.patch
Description: Binary data

Re: Ltree: set of allowed charcters is limited to [A-Za-z0-9_]. Could the dash "-" be included?

[joernbs]

Hello David,

I think this is a misunderstanding.
The seperator in ltree is the dot (.) , of cause I did not asked to change
that.
I asked about to expand allowed characters in the ltree-string [A-Za-z0-9_]
to [a-zA-Z0-9_/- ] including dash(-), slash(/) and whitespace( ), common
charcaters in wording or real path-names to be transformed into and from
ltree.

Jörn

Am Sa., 27. Okt. 2018 um 18:14 Uhr schrieb David G. Johnston <
david.g.johns...@gmail.com>:

> On Saturday, October 27, 2018, joernbs  wrote:
>
>> Dear friends,
>>
>> I would like to use ltree for search paths in a warehouse application,
>> something like "Material-Entry-01.Main-Aisle.Shelf-Aisle-R07/R08.R07-12-03"
>> Unfortunately I can not use common separators like dash (-) or slash(/)
>>
>> Documentation states only thes characters [A-Za-z0-9_] are allowed.
>> https://www.postgresql.org/docs/10/static/ltree.html
>>
>
> I don’t see how this would be possible to do with the existing type - too
> much potential breakage of existing data.  Your example itself shows why
> using dash as a separator is a bad idea.
>
> David J.
>
>

Re: Conflicting option checking in pg_restore

[Fabien COELHO]

Hallå Daniel,


Checking for conflicting options in pg_restore was mostly done in main() with
one check deferred until RestoreArchive().  Reading the git history makes it
seem like it simply happened, without the disjoint checking being intentional.
Am I reading it right that we can consolidate all the option checking to
main()?  The attached patch does that, and also rewords the error message to
make it similar to the other option checks.


Patch applies cleanly, compiles, both global and local "make check" ok.

As there are no test changes, this is not tested. I'd suggest to add it to 
"src/bin/pg_dump/t/001_basic.pl".


There is a possible catch:

Function RestoreArchive is called both from pg_dump & pg_restore, so now 
the sanity check is not performed for the former (which does not have the 
-1 option, though). Moreover, the function is noted "Public", which may 
suggest that external tools could take advantage of it, and if so it 
suggests that maybe it is not wise to remove the test. Any opinion around?


Maybe the check could be kept in RestoreArchive with a comment to say it 
is redundant, but the check could be performed in pg_restore as well for 
the sake of having an better and more homogeneous error message.


--
Fabien.