Re: pg13 PGDLLIMPORT list

[Julien Rouhaud]

On Sat, 18 Jan 2020, 09:04 Amit Kapila  On Sat, Jan 18, 2020 at 7:56 AM Michael Paquier 
> wrote:
> >
> > On Fri, Jan 17, 2020 at 03:07:48PM -0700, legrand legrand wrote:
> > > Would it be possible to add PGDLLIMPORT to permit to build following
> > > extensions on windows
> > >
> > > pg_stat_sql_plans:
> > > src/include/pgstat.h
> > > extern PGDLLIMPORT bool pgstat_track_activities;
> > >
> > > pg_background:
> > > src/include/storage/proc.h
> > > extern PGDLLIMPORT intStatementTimeout;
> >
> > No objections from me to add both to what's imported.
> >
>
> +1 for adding PGDLLIMPORT to these variables.  In the past, we have
> added it on the request of some extension authors, so I don't see any
> problem doing this time as well.
>

+1 too

>

Re: pg13 PGDLLIMPORT list

[Amit Kapila]

On Sat, Jan 18, 2020 at 7:56 AM Michael Paquier  wrote:
>
> On Fri, Jan 17, 2020 at 03:07:48PM -0700, legrand legrand wrote:
> > Would it be possible to add PGDLLIMPORT to permit to build following
> > extensions on windows
> >
> > pg_stat_sql_plans:
> > src/include/pgstat.h
> > extern PGDLLIMPORT bool pgstat_track_activities;
> >
> > pg_background:
> > src/include/storage/proc.h
> > extern PGDLLIMPORT intStatementTimeout;
>
> No objections from me to add both to what's imported.
>

+1 for adding PGDLLIMPORT to these variables.  In the past, we have
added it on the request of some extension authors, so I don't see any
problem doing this time as well.

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

Re: Implementing Incremental View Maintenance

[nuko yokohama]

Hi.
I understand.
Even if the function name is min, there is a possibility that it is not an
aggregation operation for finding the minimum value, so it is restricted.
I understood  aggregation of user-defined types is a constraint.

Also, I agree with the error message improvements.

2020年1月17日(金) 17:12 Yugo NAGATA :

> On Thu, 16 Jan 2020 12:59:11 +0900
> nuko yokohama  wrote:
>
> > Aggregate operation of user-defined type cannot be specified
> > (commit e150d964df7e3aeb768e4bae35d15764f8abd284)
> >
> > A SELECT statement using the MIN() and MAX() functions can be executed
> on a
> > user-defined type column that implements the aggregate functions MIN ()
> and
> > MAX ().
> > However, if the same SELECT statement is specified in the AS clause of
> > CREATE INCREMENTAL MATERIALIZED VIEW, the following error will occur.
> >
> > ```
> > SELECT MIN(data) data_min, MAX(data) data_max FROM foo;
> >  data_min | data_max
> > --+--
> >  1/3  | 2/3
> > (1 row)
> >
> > CREATE INCREMENTAL MATERIALIZED VIEW foo_min_imv AS SELECT MIN(data)
> > data_min FROM foo;
> > psql:extension-agg.sql:14: ERROR:  aggregate function min is not
> supported
> > CREATE INCREMENTAL MATERIALIZED VIEW foo_max_imv AS SELECT MAX(data)
> > data_max FROM foo;
> > psql:extension-agg.sql:15: ERROR:  aggregate function max is not
> supported
> > ```
> >
> > Does query including user-defined type aggregate operation not supported
> by
> > INCREMENTAL MATERIALIZED VIEW?
>
> The current implementation supports only built-in aggregate functions, so
> user-defined aggregates are not supported, although it is allowed before.
> This is because we can not know how user-defined aggregates behave and if
> it can work safely with IVM. Min/Max on your fraction type may work well,
> but it is possible that some user-defined aggregate functions named min
> or max behave in totally different way than we expected.
>
> In future, maybe it is possible support user-defined aggregates are
> supported
> by extending pg_aggregate and adding support functions for IVM, but there
> is
> not still a concrete plan for now.
>
> BTW, the following error message doesn't look good because built-in min is
> supported, so I will improve it.
>
>  ERROR:  aggregate function min is not supported
>
> Regards,
> Yugo Nagata
>
> >
> > An execution example is shown below.
> >
> > ```
> > [ec2-user@ip-10-0-1-10 ivm]$ cat extension-agg.sql
> > --
> > -- pg_fraction: https://github.com/nuko-yokohama/pg_fraction
> > --
> > DROP EXTENSION IF EXISTS pg_fraction CASCADE;
> > DROP TABLE IF EXISTS foo CASCADE;
> >
> > CREATE EXTENSION IF NOT EXISTS pg_fraction;
> > \dx
> > \dT+ fraction
> >
> > CREATE TABLE foo (id int, data fraction);
> > INSERT INTO foo (id, data) VALUES (1,'2/3'),(2,'1/3'),(3,'1/2');
> > SELECT MIN(data) data_min, MAX(data) data_max FROM foo;
> > CREATE INCREMENTAL MATERIALIZED VIEW foo_min_imv AS SELECT MIN(data)
> > data_min FROM foo;
> > CREATE INCREMENTAL MATERIALIZED VIEW foo_max_imv AS SELECT MAX(data)
> > data_max FROM foo;
> >
> > SELECT MIN(id) id_min, MAX(id) id_max FROM foo;
> > CREATE INCREMENTAL MATERIALIZED VIEW foo_id_imv AS SELECT MIN(id) id_min,
> > MAX(id) id_max FROM foo;
> > ```
> >
> > Best regards.
> >
> > 2018年12月27日(木) 21:57 Yugo Nagata :
> >
> > > Hi,
> > >
> > > I would like to implement Incremental View Maintenance (IVM) on
> > > PostgreSQL.
> > > IVM is a technique to maintain materialized views which computes and
> > > applies
> > > only the incremental changes to the materialized views rather than
> > > recomputate the contents as the current REFRESH command does.
> > >
> > > I had a presentation on our PoC implementation of IVM at PGConf.eu 2018
> > > [1].
> > > Our implementation uses row OIDs to compute deltas for materialized
> > > views.
> > > The basic idea is that if we have information about which rows in base
> > > tables
> > > are contributing to generate a certain row in a matview then we can
> > > identify
> > > the affected rows when a base table is updated. This is based on an
> idea of
> > > Dr. Masunaga [2] who is a member of our group and inspired from
> ID-based
> > > approach[3].
> > >
> > > In our implementation, the mapping of the row OIDs of the materialized
> view
> > > and the base tables are stored in "OID map". When a base relation is
> > > modified,
> > > AFTER trigger is executed and the delta is recorded in delta tables
> using
> > > the transition table feature. The accual udpate of the matview is
> triggerd
> > > by REFRESH command with INCREMENTALLY option.
> > >
> > > However, we realize problems of our implementation. First, WITH OIDS
> will
> > > be removed since PG12, so OIDs are no longer available. Besides this,
> it
> > > would
> > > be hard to implement this since it needs many changes of executor
> nodes to
> > > collect base tables's OIDs during execuing a query. Also, the cost of
> > > maintaining
> > > OID map would be high.
> > >
> > > For these reasons, we started to think to 

Re: Reorderbuffer crash during recovery

[Amit Kapila]

On Sat, Jan 18, 2020 at 2:42 AM Alvaro Herrera  wrote:
>
> On 2020-Jan-17, vignesh C wrote:
>
> > Thanks Dilip for reviewing.
> > I have fixed the comments you have suggested.
>
> I ended up rewording that comment completely; I thought the original was
> not explaining things well enough.
>
> I also changed the comment for final_lsn in reorderbuffer.h: not only I
> remove the line that I added in df9f682c7bf8 (the previous bugfix), but
> I also remove the line that says "error during decoding", which came in
> with the very first logical decoding commit (b89e151054a); I couldn't
> find any evidence that final_lsn is being set on errors of any kind
> (other than transaction abort, which doesn't seem an "error" in that
> sense.)
>
> Please give these comments a read; maybe I have misunderstood something
> and my comment is wrong.
>

The comments added by you look correct and good to me.  Thanks.

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

Re: reduce size of fmgr_builtins array

[John Naylor]

On Tue, Jan 7, 2020 at 9:08 PM Heikki Linnakangas  wrote:
> Yeah. Nevertheless, it would be nice to be able to demonstrate the
> benefit in some test, at least. It feels hard to justify committing a
> performance patch if we can't show the benefit. Otherwise, we should
> just try to keep it as simple as possible, to optimize for readability.
>
> A similar approach was actually discussed a couple of years back:
> https://www.postgresql.org/message-id/bd13812c-c4ae-3788-5b28-5633beed2929%40iki.fi.
> The conclusion then was that it's not worth the trouble or the code
> complication. So I think this patch is Rejected, unless you can come up
> with a test case that concretely shows the benefit.

Thanks for reviewing! As expected, a microbenchmark didn't show a
difference. I could try profiling in some workload, but I don't think
the benefit would be worth the effort involved. I've marked it
rejected.

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

Re: PATCH: standby crashed when replay block which truncated in standby but failed to truncate in master node

[Michael Paquier]

On Fri, Jan 17, 2020 at 07:36:51PM +0900, Fujii Masao wrote:
> On Fri, Jan 17, 2020 at 1:47 PM Michael Paquier  wrote:
>> Thanks.  I have few tweaks to propose to the docs.
>>
>> +raise a PANIC-level error, aborting the recovery. Setting
>> Instead of "PANIC-level error", I would just use "PANIC error", and
> 
> I have no strong opinion about this, but I used "PANIC-level error"
> because the description for data_sync_retry has already used it.

Okay.  Fine with what you think is good.

>> instead of "aborting the recovery" just "crashing the server".
> 
> PANIC implies server crash, so IMO "crashing the server" is
> a bit redundant, and "aborting the recovery" is better because
> "continue the recovery" is used later.

Okay.  I see your point here.

> So, what about
> 
> ---
> causes the system to ignore invalid page references in WAL records
> (but still report a warning), and continue the recovery.
> ---

And that sounds good to me.  Switching the patch as ready for
committer.
--
Michael


signature.asc
Description: PGP signature

Re: Use compiler intrinsics for bit ops in hash

[John Naylor]

On Wed, Jan 15, 2020 at 6:09 AM David Fetter  wrote:
> [v2 patch]

Hi David,

I have a stylistic comment on this snippet:

- for (i = _hash_log2(metap->hashm_bsize); i > 0; --i)
- {
- if ((1 << i) <= metap->hashm_bsize)
- break;
- }
+ i =  pg_leftmost_one_pos32(metap->hashm_bsize);
  Assert(i > 0);
  metap->hashm_bmsize = 1 << i;
  metap->hashm_bmshift = i + BYTE_TO_BIT;

Naming the variable "i" made sense when it was a loop counter, but it
seems out of place now. Same with the Assert.

Also, this

+ * using BSR where available */

is not directly tied to anything in this function, or even in the
function it calls, and could get out of date easily.

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

Re: Improve errors when setting incorrect bounds for SSL protocols

[Michael Paquier]

On Thu, Jan 16, 2020 at 10:00:52AM +0900, Michael Paquier wrote:
> Thanks for the review.  Let's wait a couple of days to see if others
> have objections or more comments about this patch, but I'd like to
> fix the issue and backpatch down to 12 where the parameters have been
> introduced.

And committed.
--
Michael


signature.asc
Description: PGP signature

Re: Expose lock group leader pid in pg_stat_activity

[Michael Paquier]

On Fri, Jan 17, 2020 at 05:07:55PM +0100, Julien Rouhaud wrote:
> Oh indeed.  But unless we hold some LWLock during the whole function
> execution, we cannot guarantee a consistent view right?

Yep.  That's possible.

> And isn't it already possible to e.g. see a parallel worker in
> pg_stat_activity while all other queries are shown are idle, if
> you're unlucky enough?

Yep.  That's possible.

> Also, LockHashPartitionLockByProc requires the leader PGPROC, and
> there's no guarantee that we'll see the leader before any of the
> workers, so I'm unsure how to implement what you said.  Wouldn't it be
> better to simply fetch the leader PGPROC after acquiring a shared
> ProcArrayLock, and using that copy to display the pid, after checking
> that we retrieved a non-null PGPROC?

Another idea would be to check if the current PGPROC entry is a leader
and print in an int[] the list of PIDs of all the workers while
holding a shared LWLock to avoid anything to be unregistered.  Less
handy, but a bit more consistent.  One problem with doing that is
that you may have in your list of PIDs some worker processes that are
already gone when going through all the other backend entries.  An
advantage is that an empty array could mean "I am the leader, but
nothing has been registered yet to my group lock." (note that the
leader adds itself to lockGroupMembers).
--
Michael


signature.asc
Description: PGP signature

Re: pg13 PGDLLIMPORT list

[Michael Paquier]

On Fri, Jan 17, 2020 at 03:07:48PM -0700, legrand legrand wrote:
> Would it be possible to add PGDLLIMPORT to permit to build following
> extensions on windows
> 
> pg_stat_sql_plans:
> src/include/pgstat.h
> extern PGDLLIMPORT bool pgstat_track_activities;
> 
> pg_background:
> src/include/storage/proc.h
> extern PGDLLIMPORT intStatementTimeout;

No objections from me to add both to what's imported.  Do you have a 
specific use-case in mind for an extension on Windows?  Just
wondering..
--
Michael


signature.asc
Description: PGP signature

Re: Amcheck: do rightlink verification with lock coupling

[Peter Geoghegan]

On Thu, Jan 16, 2020 at 5:11 PM Peter Geoghegan  wrote:
> I find this argument convincing. I'll try to get this committed soon.
>
> While you could have used bt_index_parent_check() or heapallindexed to
> detect the issue, those two options are a lot more expensive (plus the
> former option won't work on a standby). Relaxing the principle that
> says that we shouldn't hold multiple buffer locks concurrently doesn't
> seem like that much to ask for to get such a clear benefit.

Having looked into it some more, I now have my doubts about this
patch. REDO routine code like btree_xlog_split() and
btree_xlog_unlink_page() feel entitled to only lock one page at a
time, which invalidates the assumption that we can couple locks on the
leaf level to verify mutual agreement in left and right sibling links
(with only an AccessShareLock on bt_index_check()'s target index
relation). It would definitely be safe for bt_index_check() to so this
were it not running in recovery mode, but that doesn't seem very
useful on its own.

I tried to come up with a specific example of how this could be
unsafe, but my explanation was all over the place (this could have had
something to do with it being Friday evening). Even still, it's up to
the patch to justify why it's safe, and that seems even more
difficult.

--
Peter Geoghegan

Re: longs where uint64s could be

[Peter Geoghegan]

On Fri, Jan 17, 2020 at 4:42 PM David Fetter  wrote:
> While going over places where I might use compiler intrinsics for
> things like ceil(log2(n))) and next power of 2(n), I noticed that a
> lot of things that can't be fractional are longs instead of, say,
> uint64s. Is this the case for historical reasons, or is there some
> more specific utility to expressing as longs things that can only have
> non-negative integer values? Did this practice pre-date our
> now-required 64-bit integers?

Yeah, it's historic. I wince when I see "long" integers. They're
almost wrong by definition. Windows has longs that are only 32-bits
wide/the same width as a regular "int". Anybody that uses a long must
have done so because they expect it to be wider than an int, even
though in general it cannot be assumed to be in Postgres C code.

work_mem calculations often use long by convention. We restrict the
size of work_mem on Windows in order to make this safe everywhere. I
believe that this is based on a tacit assumption that long is wider
outside of Windows.

logtape.c uses long ints. This means that Windows cannot support very
large external sorts. I don't recall hearing any complaints about
that, but it still doesn't seem great.

-- 
Peter Geoghegan

longs where uint64s could be

[David Fetter]

Folks,

While going over places where I might use compiler intrinsics for
things like ceil(log2(n))) and next power of 2(n), I noticed that a
lot of things that can't be fractional are longs instead of, say,
uint64s. Is this the case for historical reasons, or is there some
more specific utility to expressing as longs things that can only have
non-negative integer values? Did this practice pre-date our
now-required 64-bit integers?

Thanks in advance for any insights into this!

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: jsonb_set() strictness considered harmful to data

[Rob Sargent]

> On Jan 17, 2020, at 4:28 PM, Ariadne Conill  wrote:
> 
> Hello,
> 
> January 17, 2020 5:21 PM, "Tomas Vondra"  > wrote:
> 
> Thank you very much for coming together and finding a solution to this bug!
> 
> Ariadne
Let’s leave it at “issue” :)

Re: jsonb_set() strictness considered harmful to data

[Ariadne Conill]

Hello,

January 17, 2020 5:21 PM, "Tomas Vondra"  wrote:

> On Wed, Jan 08, 2020 at 05:24:05PM +1030, Andrew Dunstan wrote:
> 
>> On Wed, Jan 8, 2020 at 7:08 AM Pavel Stehule  wrote:
>>> Hi
>>> 
>>> po 6. 1. 2020 v 22:34 odesílatel Andrew Dunstan 
>>>  napsal:
>> 
>> Updated version including docco and better error message.
>> 
>> cheers
>> 
>> andrew
>>> I think so my objections are solved. I have small objection
>>> 
>>> + errdetail("exception raised due to \"null_value_treatment := 
>>> 'raise_exception'\""),
>>> + errhint("to avoid, either change the null_value_treatment argument or 
>>> ensure that an SQL NULL is
>>> not used")));
>>> 
>>> "null_value_treatment := 'raise_exception'\""
>>> 
>>> it use proprietary PostgreSQL syntax for named parameters. Better to use 
>>> ANSI/SQL syntax
>>> 
>>> "null_value_treatment => 'raise_exception'\""
>>> 
>>> It is fixed in attached patch
>>> 
>>> source compilation without warnings,
>>> compilation docs without warnings
>>> check-world passed without any problems
>>> 
>>> I'll mark this patch as ready for commiter
>>> 
>>> Thank you for your work
>> 
>> Thanks for the review. I propose to commit this shortly.
> 
> Now that this was committed, I've updated the patch status accordingly.

Thank you very much for coming together and finding a solution to this bug!

Ariadne

Re: jsonb_set() strictness considered harmful to data

[Tomas Vondra]

On Wed, Jan 08, 2020 at 05:24:05PM +1030, Andrew Dunstan wrote:

On Wed, Jan 8, 2020 at 7:08 AM Pavel Stehule  wrote:


Hi

po 6. 1. 2020 v 22:34 odesílatel Andrew Dunstan 
 napsal:



Updated version including docco and better error message.

cheers

andrew



I think so my objections are solved. I have small objection

+ errdetail("exception raised due to \"null_value_treatment := 
'raise_exception'\""),
+ errhint("to avoid, either change the null_value_treatment argument or ensure that 
an SQL NULL is not used")));

"null_value_treatment := 'raise_exception'\""

it use proprietary PostgreSQL syntax for named parameters. Better to use 
ANSI/SQL syntax

"null_value_treatment => 'raise_exception'\""

It is fixed in attached patch

source compilation without warnings,
compilation docs without warnings
check-world passed without any problems

I'll mark this patch as ready for commiter

Thank you for your work




Thanks for the review. I propose to commit this shortly.



Now that this was committed, I've updated the patch status accordingly.

Thanks!

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

Re: Decade indication

[Bruce Momjian]

On Thu, Jan  2, 2020 at 08:52:17AM -0500, Tom Lane wrote:
> Robert Haas  writes:
> > On Wed, Jan 1, 2020 at 11:01 PM Tom Lane  wrote:
> >> I see Randall Munroe has weighed in on this topic:
> >> https://xkcd.com/2249/
> 
> > And the conclusion is ... the whole discussion is stupid?
> 
> Well, it's not terribly useful anyway.  Arguments founded on an
> assumption that there's anything rational or consistent about
> human calendars tend to run into problems with that assumption.

I assume there is enough agreement that decades start on 20X0 that we
don't need to document that Postgres does that.

-- 
  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 +

Re: [WIP] UNNEST(REFCURSOR): allowing SELECT to consume data from a REFCURSOR

[Dent John]

> On 14 Jan 2020, at 14:53, Daniel Verite  wrote:
> 
> What is the expected result anyway? A single column with a "record"
> type? FWIW I notice that with plpgsql, this is not allowed to happen:

Hmm. How interesting.

I had not really investigated what happens in the case of a function returning 
SETOF (untyped) RECORD in a SELECT clause because, whatever the result, there’s 
no mechanism to access the individual fields.

As you highlight, it doesn’t work at all in plpgsql, and plperl is the same.

However, SQL language functions get away with it. For example, inspired by 
_pg_expandarray():

CREATE OR REPLACE FUNCTION public.my_pg_expandarray(anyarray)
RETURNS SETOF record
LANGUAGE sql
IMMUTABLE PARALLEL SAFE STRICT
AS $function$
select $1[s], s - pg_catalog.array_lower($1,1) + 1
from pg_catalog.generate_series(pg_catalog.array_lower($1,1),
pg_catalog.array_upper($1,1), 1) as g(s)
$function$

postgres=# select my_pg_expandarray (array[0, 1, 2, 3, 4]);
 my_pg_expandarray 
---
 (0,1)
 (1,2)
 (2,3)
 (3,4)
 (4,5)
(5 rows)

Back in the FROM clause, it’s possible to manipulate the individual fields:

postgres=# select b, a from my_pg_expandarray (array[0, 1, 2, 3, 4]) as r(a 
int, b int);
 b | a 
---+---
 1 | 0
 2 | 1
 3 | 2
 4 | 3
 5 | 4
(5 rows)

It’s quite interesting. All the other PLs make explicit checks for 
rsinfo.expectedDesc being non-NULL, but fmgr_sql() explicitly calls out the 
contrary: “[…] note we do not require caller to provide an expectedDesc.” So I 
guess either there’s something special about the SQL PL, or perhaps the other 
PLs are just inheriting a pattern of being cautious.

Either way, though, there’s no way that I can see to "get at” the fields inside 
the anonymous record that is returned when the function is in the SELECT list.

But back to the failure, I still need to make it not crash. I guess it doesn’t 
matter whether I simply refuse to work if called from the SELECT list, or just 
return an anonymous record, like fmgr_sql() does.

d.

Re: Patch to document base64 encoding

[Tom Lane]

Alvaro Herrera  writes:
> Tom, you're marked as committer for this one in the commitfest app; are
> you still intending to get it committed?  If not, I can.

I've not been paying much attention to this thread, but I'll take
another look.

regards, tom lane

Re: Avoid full GIN index scan when possible

[Alexander Korotkov]

On Sat, Jan 18, 2020 at 12:48 AM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Wed, Jan 15, 2020 at 2:03 AM Tom Lane  wrote:
> >> Hmm ... yeah, these test cases are not large enough to exercise any
> >> lossy-page cases, are they?  I doubt we should try to make a new regression
> >> test that is that big.  (But if there is one already, maybe we could add
> >> more test queries with it, instead of creating whole new tables?)
>
> > I've checked that none of existing tests for GIN can produce lossy
> > bitmap page with minimal work_mem = '64kB'.  I've tried to generate
> > sample table with single integer column to get lossy page.  It appears
> > that we need at least 231425 rows to get it.  With wider rows, we
> > would need less number of rows, but I think total heap size wouldn't
> > be less.
> > So, I think we don't need so huge regression test to exercise this corner 
> > case.
>
> Ugh.  Yeah, I don't want a regression test case that big either.
>
> v15 looks good to me.

Thanks! Pushed!

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

pg13 PGDLLIMPORT list

[legrand legrand]

Hello,
would it be possible to add PGDLLIMPORT to permit to build following
extensions on windows

pg_stat_sql_plans:
src/include/pgstat.h
extern PGDLLIMPORT bool pgstat_track_activities;

pg_background:
src/include/storage/proc.h
extern PGDLLIMPORT int  StatementTimeout;

Thanks in advance
Regards
PAscal



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

Re: Avoid full GIN index scan when possible

[Tom Lane]

Alexander Korotkov  writes:
> On Wed, Jan 15, 2020 at 2:03 AM Tom Lane  wrote:
>> Hmm ... yeah, these test cases are not large enough to exercise any
>> lossy-page cases, are they?  I doubt we should try to make a new regression
>> test that is that big.  (But if there is one already, maybe we could add
>> more test queries with it, instead of creating whole new tables?)

> I've checked that none of existing tests for GIN can produce lossy
> bitmap page with minimal work_mem = '64kB'.  I've tried to generate
> sample table with single integer column to get lossy page.  It appears
> that we need at least 231425 rows to get it.  With wider rows, we
> would need less number of rows, but I think total heap size wouldn't
> be less.
> So, I think we don't need so huge regression test to exercise this corner 
> case.

Ugh.  Yeah, I don't want a regression test case that big either.

v15 looks good to me.

regards, tom lane

Re: Crash in BRIN summarization

[Alvaro Herrera]

On 2019-Aug-28, Heikki Linnakangas wrote:

> I bumped into a little bug in BRIN, while hacking on something unrelated.
> This causes a segfault, or an assertion failure if assertions are enabled:

Heikki, I just noticed that you haven't pushed this bugfix.  Would you
like me to?  (If I don't hear from you, I'll probably get to it next
week.)

Thanks

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Avoid full GIN index scan when possible

[Alexander Korotkov]

On Sat, Jan 18, 2020 at 12:33 AM Alexander Korotkov
 wrote:
> So, I think we don't need so huge regression test to exercise this corner 
> case.

Forgot to mention.  I'm going to push v15 if no objections.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company

Re: Avoid full GIN index scan when possible

[Alexander Korotkov]

On Wed, Jan 15, 2020 at 2:03 AM Tom Lane  wrote:
> Alexander Korotkov  writes:
> > On Tue, Jan 14, 2020 at 9:43 PM Tom Lane  wrote:
> >> One thing I'm still not happy about is the comment in
> >> collectMatchesForHeapRow.  v12 failed to touch that at all, so I tried to
> >> fill it in, but I'm not sure if my explanation is good.
>
> > I've tried to rephrase this comment making it better from my point of
> > view.  It's hard for me to be sure about this, since I'm not native
> > English speaker.  I'd like you to take a look on it.
>
> Yeah, that's not great as-is.  Maybe like
>
> +* All scan keys except excludeOnly require at least one entry to 
> match.
> +* excludeOnly keys are an exception, because their implied
> +* GIN_CAT_EMPTY_QUERY scanEntry always matches.  So return "true"
> +* if all non-excludeOnly scan keys have at least one match.

Looks good to me.

> >> Also, if we know
> >> that excludeOnly keys are going to be ignored, can we save any work in
> >> the main loop of that function?
>
> > It doesn't look so for me.  We still need to collect matches for
> > consistent function call afterwards.
>
> Ah, right.
>
> > I also had concerns about how excludeOnly keys work with lossy pages.
> > I didn't find exact error.  But I've added code, which skips
> > excludeOnly keys checks for lossy pages.  They aren't going to exclude
> > any lossy page anyway.  So, we can save some resources by skipping
> > this.
>
> Hmm ... yeah, these test cases are not large enough to exercise any
> lossy-page cases, are they?  I doubt we should try to make a new regression
> test that is that big.  (But if there is one already, maybe we could add
> more test queries with it, instead of creating whole new tables?)

I've checked that none of existing tests for GIN can produce lossy
bitmap page with minimal work_mem = '64kB'.  I've tried to generate
sample table with single integer column to get lossy page.  It appears
that we need at least 231425 rows to get it.  With wider rows, we
would need less number of rows, but I think total heap size wouldn't
be less.

So, I think we don't need so huge regression test to exercise this corner case.

--
Alexander Korotkov
Postgres Professional: http://www.postgrespro.com
The Russian Postgres Company


0001-Avoid-GIN-full-scan-for-empty-ALL-keys-v15.patch
Description: Binary data

Re: making the backend's json parser work in frontend code

[Robert Haas]

On Thu, Jan 16, 2020 at 8:55 PM Andrew Dunstan
 wrote:
> I'm probably responsible for a good deal of the mess, so let me say Thankyou.
>
> I'll have a good look at these.

Thanks, appreciated.

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

Re: Patch to document base64 encoding

[Alvaro Herrera]

Tom, you're marked as committer for this one in the commitfest app; are
you still intending to get it committed?  If not, I can.

Thanks,

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: Reorderbuffer crash during recovery

[Alvaro Herrera]

On 2020-Jan-17, vignesh C wrote:

> Thanks Dilip for reviewing.
> I have fixed the comments you have suggested.

I ended up rewording that comment completely; I thought the original was
not explaining things well enough.

I also changed the comment for final_lsn in reorderbuffer.h: not only I
remove the line that I added in df9f682c7bf8 (the previous bugfix), but
I also remove the line that says "error during decoding", which came in
with the very first logical decoding commit (b89e151054a); I couldn't
find any evidence that final_lsn is being set on errors of any kind
(other than transaction abort, which doesn't seem an "error" in that
sense.)

Please give these comments a read; maybe I have misunderstood something
and my comment is wrong.

Pushed now to all branches.  Thanks, Vignesh, Amit and Dilip.

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: proposal: schema variables

[Pavel Stehule]

Hi

po 30. 12. 2019 v 21:05 odesílatel Pavel Stehule 
napsal:

> Hi
>
> po 30. 12. 2019 v 17:27 odesílatel Philippe BEAUDOIN 
> napsal:
>
>> The following review has been posted through the commitfest application:
>> make installcheck-world:  tested, passed
>> Implements feature:   tested, passed
>> Spec compliant:   not tested
>> Documentation:tested, failed
>>
>> Hi Pavel,
>>
>> I have tested the latest version of your patch.
>> Both issues I reported are now fixed. And you largely applied my
>> proposals. That's great !
>>
>> I have also spent some time to review more closely the documentation. I
>> will send you a direct mail with an attached file for some minor comments
>> on this topic.
>>
>> Except these documentation remarks to come, I haven't any other issue or
>> suggestion to report.
>> Note that I have not closely looked at the C code itself. But may be some
>> other reviewers have already done that job.
>> If yes, my feeling is that the patch could soon be set as "Ready for
>> commiter".
>>
>> Best regards. Philippe.
>>
>> The new status of this patch is: Waiting on Author
>>
>
> Thank you very much for your comments, and notes. Updated patch attached.
>

rebase



> Regards
>
> Pavel
>
>


schema-variables-20200117.patch.gz
Description: application/gzip

Re: Implementing Incremental View Maintenance

[legrand legrand]

Hello,

It seems that patch v11 doesn't apply any more.
Problem with "scanRTEForColumn" maybe because of change:

https://git.postgresql.org/pg/commitdiff/b541e9accb28c90656388a3f827ca3a68dd2a308

Regards
PAscal



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

Re: psql - add SHOW_ALL_RESULTS option

[Daniel Verite]

Tom Lane wrote:

> I'm not really holding my breath for that to happen, considering
> it would involve fundamental breakage of the wire protocol.
> (For example, extended query protocol assumes that Describe
> Portal only needs to describe one result set.  There might be
> more issues, but that one's bad enough.)

I'm not sure that CALL can be used at all with the extended protocol
today  (just like multiple queries per statements, except that for these,
I'm sure).
My interpretation is that the extended protocol deliberately
lefts out the possibility of multiple result sets because it doesn't fit
with how it's designed and if you want to have this, you can just use
the old protocol's Query message. There is no need to break anything
or invent anything but on the contrary to use the older way.

Considering these 3 ways to use libpq to send queries:

1. using old protocol with PQexec: only one resultset.

2. using old protocol with PQsendQuery+looping on PQgetResult:
same as #1 except multiple result sets can be processed

3. using extended protocol: not for multiple result sets, not for copy,
possibly not for other things, but  can use bind parameters, binary format,
pipelining,...

The current patch is about using #2 instead of #1.
They have been patches about doing bits of #3 in some cases
(binary output, maybe parameters too?) and none got eventually in.
ISTM that the current situation is that psql is stuck at #1 since forever
so it's not fully using the capabilities of the protocol, both old and new.


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: Parallel leader process info in EXPLAIN

[Melanie Plageman]

Both patches aren't applying cleanly anymore.
The first patch in the set applied cleanly for me before b925a00f4ef65

It mostly seems like the default settings for the patch program were
my problem, but, since I noticed that the patch tester bot was failing
to apply it also, I thought I would suggest rebasing it.

I applied it to the sha before b925a00f4ef65 and then cherry-picked it
to master and it applied fine. I attached that rebased patch with a
new version number (is that the preferred way to indicate that it is
newer even if it contains no new content?).

The second patch in the set needed a bit more looking at to rebase,
which I didn't do yet.

I played around with the first patch in the patchset and very much
appreciate seeing the leaders contribution.
However, I noticed that there were no EXPLAIN diffs in any test files
and just wondered if this was a conscious choice (even with xxx the
actual numbers, I would have thought that there would be an EXPLAIN
VERBOSE with leader participation somewhere in regress).

-- 
Melanie Plageman


0001-Show-parallel-leader-stats-in-EXPLAIN-output-v4.patch
Description: Binary data

Re: Let people set host(no)ssl settings from initdb

[David Fetter]

On Fri, Jan 17, 2020 at 08:47:49PM +0100, David Fetter wrote:
> On Wed, Jan 08, 2020 at 02:53:47AM +, tsunakawa.ta...@fujitsu.com wrote:
> > From: David Fetter 
> > > > But I see two problems with the proposed approach: (1) initdb
> > > > doesn't support setting up SSL, so the only thing you can achieve
> > > > here is to reject all TCP/IP connections, until you have set up SSL.
> > > 
> > > I don't believe any special setup is needed to require TLS for the
> > > connection, which is what this patch handles in a straightforward way.
> > 
> > I think this feature can be useful because it's common to reject remote 
> > non-TLS connections.  Eliminating the need to script for pg_hba.conf is 
> > welcome.  Setting GUC parameters just after initdb is relatively easy, 
> > because we can simply add lines at the end of postgresql.conf.  But 
> > pg_hba.conf is not because the first matching entry is effective.
> > 
> > In terms of rejecting non-secure remote connections, should 
> > hostgssenc/hostnogssenc also be handled similarly?
> 
> Yes, and they are in the enclosed patch.
> 
> > > > (2) The default pg_hba.conf only covers localhost connections.
> > > 
> > > As of this patch, it can be asked to cover all connections.
> > 
> > +  --auth-hostssl= > class="parameter">authmethod
> > +  
> > +   
> > +This option specifies the authentication method for users via
> > fg
> > +TLS connections used in pg_hba.conf
> > +(hostssl lines).
> > +   
> > +  
> > 
> > The relationship between --auth/--auth-local/--auth-host and 
> > --auth-hostssl/--auth-hostnossl is confusing.  The former is for local 
> > connections, and the latter is for remote ones.  Can we just add "remote" 
> > in the above documentation?
> 
> Done.
> 
> > Plus, you're adding the first option to initdb that handles remote 
> > connections.  As the following execution shows, it doesn't warn about using 
> > "trust" for remote connections.
> > 
> > 
> > $ initdb --auth=md5 --pwprompt --auth-hostssl=trust --auth-hostnossl=trust
> > ...
> > syncing data to disk ... ok
> > 
> > Success. You can now start the database server using:
> > 
> > pg_ctl -D /tuna/pg2 -l logfile start
> > 
> > 
> > 
> > I think we should emit a warning message like the following existing one:
> > 
> > --
> > initdb: warning: enabling "trust" authentication for local connections
> > You can change this by editing pg_hba.conf or using the option -A, or
> > --auth-local and --auth-host, the next time you run initdb.
> > -
> > initdb: warning: enabling "trust" authentication 
> 
> Done.
> 
> Best,
> David.

This time, with the patch attached.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate
>From 9dbe5b8a0e4a452367d3e86792111c2950c7b379 Mon Sep 17 00:00:00 2001
From: David Fetter 
Date: Wed, 11 Dec 2019 18:55:03 -0800
Subject: [PATCH v3] Enable setting pg_hba.conf permissions from initdb
To: hackers
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="2.24.1"

This is a multi-part message in MIME format.
--2.24.1
Content-Type: text/plain; charset=UTF-8; format=fixed
Content-Transfer-Encoding: 8bit


The new optional arguments are --auth-hostssl, --auth-hostnossl,
--auth-hostgssenc, and --auth-hostnogssenc.

diff --git a/doc/src/sgml/ref/initdb.sgml b/doc/src/sgml/ref/initdb.sgml
index da5c8f5307..4126880bcb 100644
--- a/doc/src/sgml/ref/initdb.sgml
+++ b/doc/src/sgml/ref/initdb.sgml
@@ -154,6 +154,50 @@ PostgreSQL documentation
   
  
 
+ 
+  --auth-hostssl=authmethod
+  
+   
+This option specifies the authentication method for users via
+TLS remote connections used in pg_hba.conf
+(hostssl lines).
+   
+  
+ 
+
+ 
+  --auth-hostnossl=authmethod
+  
+   
+This option specifies the authentication method for users via
+non-TLS remote connections used in pg_hba.conf
+(hostnossl lines).
+   
+  
+ 
+
+ 
+  --auth-hostgssenc=authmethod
+  
+   
+This option specifies the authentication method for users via
+encrypted GSSAPI remote connections used in pg_hba.conf
+(hostgssenc lines).
+   
+  
+ 
+
+ 
+  --auth-hostnogssenc=authmethod
+  
+   
+This option specifies the authentication method for users via
+remote connections not using encrypted GSSAPI in pg_hba.conf
+(hostnogssenc lines).
+   
+  
+ 
+
  
   --auth-local=authmethod
   
diff --git a/src/backend/libpq/pg_hba.conf.sample b/src/backend/libpq/pg_hba.conf.sample
index c853e36232..7d5189dd8f 100644
--- a/src/backend/libpq/pg_hba.conf.sample
+++ b/src/backend/libpq/pg_hba.conf.sample
@@ -87,3 +87,15 @@ hostall all ::1/128 

Re: Let people set host(no)ssl settings from initdb

[David Fetter]

On Wed, Jan 08, 2020 at 02:53:47AM +, tsunakawa.ta...@fujitsu.com wrote:
> From: David Fetter 
> > > But I see two problems with the proposed approach: (1) initdb
> > > doesn't support setting up SSL, so the only thing you can achieve
> > > here is to reject all TCP/IP connections, until you have set up SSL.
> > 
> > I don't believe any special setup is needed to require TLS for the
> > connection, which is what this patch handles in a straightforward way.
> 
> I think this feature can be useful because it's common to reject remote 
> non-TLS connections.  Eliminating the need to script for pg_hba.conf is 
> welcome.  Setting GUC parameters just after initdb is relatively easy, 
> because we can simply add lines at the end of postgresql.conf.  But 
> pg_hba.conf is not because the first matching entry is effective.
> 
> In terms of rejecting non-secure remote connections, should 
> hostgssenc/hostnogssenc also be handled similarly?

Yes, and they are in the enclosed patch.

> > > (2) The default pg_hba.conf only covers localhost connections.
> > 
> > As of this patch, it can be asked to cover all connections.
> 
> +  --auth-hostssl= class="parameter">authmethod
> +  
> +   
> +This option specifies the authentication method for users via
> fg
> +TLS connections used in pg_hba.conf
> +(hostssl lines).
> +   
>   +  
> 
> The relationship between --auth/--auth-local/--auth-host and 
> --auth-hostssl/--auth-hostnossl is confusing.  The former is for local 
> connections, and the latter is for remote ones.  Can we just add "remote" in 
> the above documentation?

Done.

> Plus, you're adding the first option to initdb that handles remote 
> connections.  As the following execution shows, it doesn't warn about using 
> "trust" for remote connections.
> 
> 
> $ initdb --auth=md5 --pwprompt --auth-hostssl=trust --auth-hostnossl=trust
> ...
> syncing data to disk ... ok
> 
> Success. You can now start the database server using:
> 
> pg_ctl -D /tuna/pg2 -l logfile start
> 
> 
> 
> I think we should emit a warning message like the following existing one:
> 
> --
> initdb: warning: enabling "trust" authentication for local connections
> You can change this by editing pg_hba.conf or using the option -A, or
> --auth-local and --auth-host, the next time you run initdb.
> -
> initdb: warning: enabling "trust" authentication 

Done.

Best,
David.
-- 
David Fetter  http://fetter.org/
Phone: +1 415 235 3778

Remember to vote!
Consider donating to Postgres: http://www.postgresql.org/about/donate

Re: Remove page-read callback from XLogReaderState.

[Alvaro Herrera]

On 2020-Jan-17, Heikki Linnakangas wrote:

> I changed that by adding new function, XLogBeginRead(), that repositions the
> reader, and removed the 'lsn' argument from XLogReadRecord() altogether. All
> callers except one in findLastCheckPoint() pg_rewind.c positioned the reader
> once, and then just read sequentially from there, so I think that API is
> more convenient.

I like it.  +1

-- 
Álvaro Herrerahttps://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Re: psql - add SHOW_ALL_RESULTS option

[Tom Lane]

"Daniel Verite"  writes:
> Yes. For instance if the stored procedures support gets improved to
> produce several result sets, how is psql going to benefit from it
> while sticking to the old way (PGresult *r = PQexec(query))
> of executing queries that discards N-1 out of N result sets?

I'm not really holding my breath for that to happen, considering
it would involve fundamental breakage of the wire protocol.
(For example, extended query protocol assumes that Describe
Portal only needs to describe one result set.  There might be
more issues, but that one's bad enough.)

When and if we break all the things that would break, it'd be
time enough for incompatible changes in psql's behavior.

regards, tom lane

Re: psql - add SHOW_ALL_RESULTS option

[Daniel Verite]

Alvaro Herrera wrote:

> if this patch enables other psql features, it might be a good step
> forward.

Yes. For instance if the stored procedures support gets improved to
produce several result sets, how is psql going to benefit from it
while sticking to the old way (PGresult *r = PQexec(query))
of executing queries that discards N-1 out of N result sets?


Best regards,
-- 
Daniel Vérité
PostgreSQL-powered mailer: http://www.manitou-mail.org
Twitter: @DanielVerite

Re: Patch to document base64 encoding

[Karl O. Pinc]

On Thu, 16 Jan 2020 15:44:44 -0300
Alvaro Herrera  wrote:

> Because of how the new tables look in the PDF docs, I thought it might
> be a good time to research how to make each function-entry occupy two
> rows: one for prototype, return type and description, and the other
> for the example and its result. 

Another approach might be to fix/change the software that generates
PDFs.  Or whatever turns it into latex if that's the
intermediate and really where the problem lies.  (FWIW, I've had luck
with dblatex.)

(Maybe best to take this thread to the pgsql-docs mailing list?)

Regards,

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

Re: BUG #16213: segfault when running a query

[Tom Lane]

I wrote:
> PG Bug reporting form  writes:
>> The above query produces an error in the server log:
>> LOG:  server process (PID 108) was terminated by signal 11: Segmentation
>> fault

> The direct cause of the crash is that by the time we get to ExecutorEnd,
> there are dangling pointers in the es_tupleTable list.

After further reflection, I'm totally dissatisfied with the quick-hack
patch I posted last night.  I think that what this example demonstrates
is that my fix (in commit 9b63c13f0) for bug #14924 in fact does not
work: the subPlan list is not the only way in which a SubPlan connects
up to the outer plan level.  I have no faith that the es_tupleTable list
is the only other way, either, or that we won't create more in future.

I think what we have to do here is revert 9b63c13f0, going back to
the previous policy of passing down parent = NULL to the transient
subexpressions, so that there is a strong guarantee that there aren't
any unwanted connections between short-lived and longer-lived state.
And then we need some other solution for making SubPlans in VALUES
lists work.  The best bet seems to be what I speculated about in that
commit message: initialize the expressions for VALUES rows that contain
SubPlans normally at executor startup, and use the trick with
short-lived expression state only for VALUES rows that don't contain any
SubPlans.  I think that the case we're worried about with long VALUES
lists is not likely to involve any SubPlans, so that this shouldn't be
too awful for memory consumption.

Another benefit of doing it like this is that SubPlans in the VALUES
lists are reported normally by EXPLAIN, while the previous hack caused
them to be missing from the output.

Objections, better ideas?

regards, tom lane

diff --git a/src/backend/executor/nodeValuesscan.c b/src/backend/executor/nodeValuesscan.c
index 8d916a0..8866121 100644
--- a/src/backend/executor/nodeValuesscan.c
+++ b/src/backend/executor/nodeValuesscan.c
@@ -26,6 +26,7 @@
 #include "executor/executor.h"
 #include "executor/nodeValuesscan.h"
 #include "jit/jit.h"
+#include "optimizer/clauses.h"
 #include "utils/expandeddatum.h"
 
 
@@ -50,7 +51,7 @@ ValuesNext(ValuesScanState *node)
 	EState	   *estate;
 	ExprContext *econtext;
 	ScanDirection direction;
-	List	   *exprlist;
+	int			curr_idx;
 
 	/*
 	 * get information from the estate and scan state
@@ -67,19 +68,11 @@ ValuesNext(ValuesScanState *node)
 	{
 		if (node->curr_idx < node->array_len)
 			node->curr_idx++;
-		if (node->curr_idx < node->array_len)
-			exprlist = node->exprlists[node->curr_idx];
-		else
-			exprlist = NIL;
 	}
 	else
 	{
 		if (node->curr_idx >= 0)
 			node->curr_idx--;
-		if (node->curr_idx >= 0)
-			exprlist = node->exprlists[node->curr_idx];
-		else
-			exprlist = NIL;
 	}
 
 	/*
@@ -90,16 +83,16 @@ ValuesNext(ValuesScanState *node)
 	 */
 	ExecClearTuple(slot);
 
-	if (exprlist)
+	curr_idx = node->curr_idx;
+	if (curr_idx >= 0 && curr_idx < node->array_len)
 	{
+		List	   *exprlist = node->exprlists[curr_idx];
+		List	   *exprstatelist = node->exprstatelists[curr_idx];
 		MemoryContext oldContext;
-		List	   *oldsubplans;
-		List	   *exprstatelist;
 		Datum	   *values;
 		bool	   *isnull;
 		ListCell   *lc;
 		int			resind;
-		int			saved_jit_flags;
 
 		/*
 		 * Get rid of any prior cycle's leftovers.  We use ReScanExprContext
@@ -109,38 +102,32 @@ ValuesNext(ValuesScanState *node)
 		ReScanExprContext(econtext);
 
 		/*
-		 * Build the expression eval state in the econtext's per-tuple memory.
-		 * This is a tad unusual, but we want to delete the eval state again
-		 * when we move to the next row, to avoid growth of memory
-		 * requirements over a long values list.
+		 * Do per-VALUES-row work in the per-tuple context.
 		 */
 		oldContext = MemoryContextSwitchTo(econtext->ecxt_per_tuple_memory);
 
 		/*
-		 * The expressions might contain SubPlans (this is currently only
-		 * possible if there's a sub-select containing a LATERAL reference,
-		 * otherwise sub-selects in a VALUES list should be InitPlans). Those
-		 * subplans will want to hook themselves into our subPlan list, which
-		 * would result in a corrupted list after we delete the eval state. We
-		 * can work around this by saving and restoring the subPlan list.
-		 * (There's no need for the functionality that would be enabled by
-		 * having the list entries, since the SubPlans aren't going to be
-		 * re-executed anyway.)
-		 */
-		oldsubplans = node->ss.ps.subPlan;
-		node->ss.ps.subPlan = NIL;
-
-		/*
-		 * As the expressions are only ever used once, disable JIT for them.
-		 * This is worthwhile because it's common to insert significant
-		 * amounts of data via VALUES().
+		 * Unless we already made the expression eval state for this row,
+		 * build it in the econtext's per-tuple memory.  This is a tad
+		 * unusual, but we want to delete the eval state again when we move to
+		 * the next row, to avoid growth of memory requirements over a long

Re: Patch to document base64 encoding

[Karl O. Pinc]

On Thu, 16 Jan 2020 14:41:33 +0100 (CET)
Fabien COELHO  wrote:
> Some comments about v13:
> 
> The note about get_byte reads:
> 
>get_byte and set_byte number the first byte of a binary string as
> byte 0. get_bit and set_bit number bits from the right within each
> byte; for example bit 0 is the least significant bit of the first
> byte, and bit 15 is the most significant bit of the second byte.
> 
> The two sentences starts with a lower case letter, which looks
> strange to me. I'd suggest to put "Functions" at the beginning of the
> sentences:
> 
>Functions get_byte and set_byte number the first byte of a binary
> string as byte 0. Functions get_bit and set_bit number bits from the
> right within each byte; for example bit 0 is the least significant
> bit of the first byte, and bit 15 is the most significant bit of the
> second byte.

Excellent suggestion, done.

> The note about hash provides an example for getting the hex
> representation out of sha*. I'd add an exemple to get the bytea
> representation from md5, eg "DECODE(MD5('hello world'), 'hex')"…

Ok.  Done.

> Maybe the encode/decode in the note could be linked to the function
> description? Well, they are just after, maybe it is not very useful.

Can't hurt?  Done.

Patch attached: doc_base64_v14.patch
Regards,

Karl 
Free Software:  "You don't pay back, you pay forward."
 -- Robert A. Heinlein
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 72072e7545..c075872364 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -1459,6 +1459,13 @@
 natively for the bit-string types.

 
+   
+ Functions which convert, both to and from, strings and
+ the bytea type
+ are documented
+ separately.
+   
+

 SQL defines some string functions that use
 key words, rather than commas, to separate
@@ -1820,101 +1827,6 @@
abcde,2,22
   
 
-  
-   
-
- convert
-
-convert(string bytea,
-src_encoding name,
-dest_encoding name)
-   
-   bytea
-   
-Convert string to dest_encoding.  The
-original encoding is specified by
-src_encoding. The
-string must be valid in this encoding.
-Conversions can be defined by CREATE CONVERSION.
-Also there are some predefined conversions. See  for available conversions.
-   
-   convert('text_in_utf8', 'UTF8', 'LATIN1')
-   text_in_utf8 represented in Latin-1
-   encoding (ISO 8859-1)
-  
-
-  
-   
-
- convert_from
-
-convert_from(string bytea,
-src_encoding name)
-   
-   text
-   
-Convert string to the database encoding.  The original encoding
-is specified by src_encoding. The
-string must be valid in this encoding.
-   
-   convert_from('text_in_utf8', 'UTF8')
-   text_in_utf8 represented in the current database encoding
-  
-
-  
-   
-
- convert_to
-
-convert_to(string text,
-dest_encoding name)
-   
-   bytea
-   
-Convert string to dest_encoding.
-   
-   convert_to('some text', 'UTF8')
-   some text represented in the UTF8 encoding
-  
-
-  
-   
-
- decode
-
-decode(string text,
-format text)
-   
-   bytea
-   
-Decode binary data from textual representation in string.
-Options for format are same as in encode.
-   
-   decode('MTIzAAE=', 'base64')
-   \x3132330001
-  
-
-  
-   
-
- encode
-
-encode(data bytea,
-format text)
-   
-   text
-   
-Encode binary data into a textual representation.  Supported
-formats are: base64, hex, escape.
-escape converts zero bytes and high-bit-set bytes to
-octal sequences (\nnn) and
-doubles backslashes.
-   
-   encode('123\000\001', 'base64')
-   MTIzAAE=
-  
-
   

 
@@ -1982,19 +1894,6 @@
4
   
 
-  
-   length(string bytea,
-encoding name )
-   int
-   
-Number of characters in string in the given
-encoding. The string
-must be valid in this encoding.
-   
-   length('jose', 'UTF8')
-   4
-  
-
   

 
@@ -2044,8 +1943,8 @@

text

-Calculates the MD5 hash of string,
-returning the result in hexadecimal
+Calculates the MD5 hash
+of string, returning the result in hexadecimal

md5('abc')
900150983cd24fb0 d6963f7d28e17f72
@@ -2358,6 +2257,66 @@
test
   
 
+  
+   
+
+ sha224
+
+sha224(string)
+   
+   bytea
+   
+SHA-224 hash
+   
+   sha224('abc')
+   

Re: Patch to document base64 encoding

[Karl O. Pinc]

On Thu, 16 Jan 2020 14:41:33 +0100 (CET)
Fabien COELHO  wrote:

> The "Binary String Functions and Operators" 9.5 section has only one 
> subsection, "9.5.1", which is about at two thirds of the page. This 
> structure looks weird. ISTM that a subsection is missing for the
> beginning of the page, or that the subsection should just be dropped,
> because it is somehow redundant with the table title.
> 
> The "9.4" section has the same structural weirdness. Either remove
> the subsection, or add some for the other parts?

Hi Fabien,

cc-ing the folks who did the work on format(), who added a sub-section
9.4.1.  The whole thread for that is here:
https://www.postgresql.org/message-id/flat/CAFj8pRBjMdAjybSZkczyez0x%3DFhC8WXvgR2wOYGuhrk1TUkraA%40mail.gmail.com

I'm going to dis-agree with you on this.  Yes, it's a little odd
to have only a single sub-section but it does not really bother me.

If there's a big/important enough chunk of information to present I like
seeing something in the table of contents.  That's the "big thing"
to my mind.

I don't see a good way to get rid of "9.4.1. format".  Adding
another sub-section heading above it just to have 2 seems
pointless.

I really want the "9.5.1 String to Binary and Binary to String
Conversion" to show up in the table of contents.  Because it
is not at all obvious that "9.5. Binary String Functions and
Operators" is the place to look for conversions between
string and binary.  Tom thought that merely having a separate
table for string<->binary functions "could be overkill"
so my impression right now is that having an entirely
separate section for these would be rejected.
(See: https://www.postgresql.org/message-id/22540.1564501...@sss.pgh.pa.us)

Otherwise an entirely separate section might be the right approach.

The following *.1 sections
in the (devel version) documentation are "single sub-sections":

(Er, this is too much but once I started I figured I'd finish.)

5.10. Inheritance
5.10.1. Caveats
9.4. String Functions and Operators
   9.4.1. format
9.30. Statistics Information Functions
9.30.1. Inspecting MCV Lists
15.4. Parallel Safety
   15.4.1. Parallel Labeling for Functions and Aggregates
17. Installation from Source Code on Windows
17.1. Building with Visual C++ or the Microsoft Windows SDK
18.10. Secure TCP/IP Connections with GSSAPI Encryption
18.10.1. Basic Setup
30.2. Subscription
30.2.1. Replication Slot Management
30.5. Architecture
30.5.1. Initial Snapshot
37.13. User-Defined Types
37.13.1. TOAST Considerations
41. Procedural Languages
41.1. Installing Procedural Languages
50.5. Planner/Optimizer
50.5.1. Generating Possible Plans
52.3. SASL Authentication
52.3.1. SCRAM-SHA-256 Authentication
57. Writing a Table Sampling Method
57.1. Sampling Method Support Functions
58.1. Creating Custom Scan Paths
58.1.1. Custom Scan Path Callbacks
58.2. Creating Custom Scan Plans
58.2.1. Custom Scan Plan Callbacks
58.3. Executing Custom Scans
   58.3.1. Custom Scan Execution Callbacks
64.4. Implementation
   64.4.1. GiST Buffering Build
67.1. Introduction
67.1.1. Index Maintenance
68.6. Database Page Layout
68.6.1. Table Row Layout
G.2. Server Applications
pg_standby — supports the creation of a PostgreSQL warm standby server
I. The Source Code Repository
I.1. Getting the Source via Git
J.4. Documentation Authoring
J.4.1. Emacs
J.5. Style Guide
J.5.1. Reference Pages

I like that I can see these in the documentation.

FYI, the format sub-section, 9.4.1, was first mentioned
by Dean Rasheed in this email:
https://www.postgresql.org/message-id/CAEZATCWLtRi-Vbh5k_2fYkOAPxas0wZh6a0brOohHtVOtHiddA%40mail.gmail.com

"I'm thinking perhaps
format() should now have its own separate sub-section in the manual,
rather than trying to cram it's docs into a single table row."

There was never really any further discussion or objection to
having a separate sub-section.

Attaching a new patch to my next email, leaving off the
folks cc-ed regarding "9.4.1 format".

Regards,

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

Re: Remove page-read callback from XLogReaderState.

[Heikki Linnakangas]

On 29/11/2019 10:14, Kyotaro Horiguchi wrote:

At Thu, 28 Nov 2019 21:37:03 +0900 (JST), Kyotaro Horiguchi 
 wrote in

0dc8ead463 hit this. Rebased.


Please review the pg_waldump.c hunks in 0001; they revert recent changes.


Ughhh! I'l check it. Thank you for noticing!!


Fixed that, re-rebased and small comment and cosmetic changes in this
version.


Thanks! I finally got around to look at this again. A lot has happened 
since I last looked at this. Notably, commit 0dc8ead463 introduced 
another callback function into the XLogReader interface. It's not 
entirely clear what the big picture with the new callback was and how 
that interacts with the refactoring here. I'll have to spend some time 
to make myself familiar with those changes.


Earlier in this thread, you wrote:


- Changed calling convention of XLogReadRecord

To make caller loop simple, XLogReadRecord now allows to specify
the same valid value while reading the a record. No longer need
to change lsn to invalid after the first call in the following
reader loop.

   while (XLogReadRecord(state, lsn, , ) == XLREAD_NEED_DATA)
   {
 if (!page_reader(state))
   break;
   }


Actually, I had also made a similar change in the patch version I posted 
at 
https://www.postgresql.org/message-id/4f7a5fad-fa04-b0a3-231b-56d200b646dc%40iki.fi. 
Maybe you missed that email? It looks like you didn't include any of the 
changes from that in the patch series. In any case, clearly that idea 
has some merit, since we both independently made a change in that 
calling convention :-).


I changed that by adding new function, XLogBeginRead(), that repositions 
the reader, and removed the 'lsn' argument from XLogReadRecord() 
altogether. All callers except one in findLastCheckPoint() pg_rewind.c 
positioned the reader once, and then just read sequentially from there, 
so I think that API is more convenient. With that, the usage looks 
something like this:


state = XLogReaderAllocate (...)
XLogBeginRead(state, start_lsn);
while (ctx->reader->EndRecPtr < end_of_wal)
{
while (XLogReadRecord(state, , ) == XLREAD_NEED_DATA)
{
if (!page_reader(state))
break;
}
/* do stuff */

}

Actually, I propose that we make that change first, and then continue 
reviewing the rest of these patches. I think it's a more convenient 
interface, independently of the callback refactoring. What do you think 
of the attached patch?


- Heikki
diff --git a/src/backend/access/transam/twophase.c b/src/backend/access/transam/twophase.c
index 6f7ee0c947d..5adf956f413 100644
--- a/src/backend/access/transam/twophase.c
+++ b/src/backend/access/transam/twophase.c
@@ -1338,7 +1338,8 @@ XlogReadTwoPhaseData(XLogRecPtr lsn, char **buf, int *len)
  errmsg("out of memory"),
  errdetail("Failed while allocating a WAL reading processor.")));
 
-	record = XLogReadRecord(xlogreader, lsn, );
+	XLogBeginRead(xlogreader, lsn);
+	record = XLogReadRecord(xlogreader, );
 	if (record == NULL)
 		ereport(ERROR,
 (errcode_for_file_access(),
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 7f4f784c0eb..882d5e8a73f 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -897,7 +897,7 @@ static void UpdateLastRemovedPtr(char *filename);
 static void ValidateXLOGDirectoryStructure(void);
 static void CleanupBackupHistory(void);
 static void UpdateMinRecoveryPoint(XLogRecPtr lsn, bool force);
-static XLogRecord *ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr,
+static XLogRecord *ReadRecord(XLogReaderState *xlogreader,
 			  int emode, bool fetching_ckpt);
 static void CheckRecoveryConsistency(void);
 static XLogRecord *ReadCheckpointRecord(XLogReaderState *xlogreader,
@@ -4246,17 +4246,17 @@ CleanupBackupHistory(void)
 }
 
 /*
- * Attempt to read an XLOG record.
+ * Attempt to read the next XLOG record.
  *
- * If RecPtr is valid, try to read a record at that position.  Otherwise
- * try to read a record just after the last one previously read.
+ * Before first call, the reader needs to be positioned to the first record
+ * by calling XLogBeginRead().
  *
  * If no valid record is available, returns NULL, or fails if emode is PANIC.
  * (emode must be either PANIC, LOG). In standby mode, retries until a valid
  * record is available.
  */
 static XLogRecord *
-ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
+ReadRecord(XLogReaderState *xlogreader, int emode,
 		   bool fetching_ckpt)
 {
 	XLogRecord *record;
@@ -4265,7 +4265,7 @@ ReadRecord(XLogReaderState *xlogreader, XLogRecPtr RecPtr, int emode,
 	/* Pass through parameters to XLogPageRead */
 	private->fetching_ckpt = fetching_ckpt;
 	private->emode = emode;
-	private->randAccess = (RecPtr != InvalidXLogRecPtr);
+	private->randAccess = (xlogreader->ReadRecPtr != InvalidXLogRecPtr);
 
 	/* This is the first attempt to read this page. */
 	lastSourceFailed = false;
@@ -4274,7 +4274,7 @@ 

Re: making the backend's json parser work in frontend code

[David Steele]

Hi Robert,

On 1/16/20 11:51 AM, Robert Haas wrote:

On Thu, Jan 16, 2020 at 1:37 PM David Steele  wrote:


So the idea here is that json.c will have the JSON SQL functions,
jsonb.c the JSONB SQL functions, and jsonapi.c the parser, and
jsonfuncs.c the utility functions?


Uh, I think roughly that, yes. Although I can't claim to fully
understand everything that's here.


Now that I've spent some time with the code I see your intent was just 
to isolate the JSON lexer code with 0002 and 0003.  As such, I now think 
they are commit-able as is.


Regards,
--
-David
da...@pgmasters.net

Re: making the backend's json parser work in frontend code

[David Steele]

Hi Robert,

On 1/16/20 11:51 AM, Robert Haas wrote:

On Thu, Jan 16, 2020 at 1:37 PM David Steele  wrote:


The next question in my mind is given the caveat that the error handing
is questionable in the front end, can we at least render/parse valid
JSON with the code?


That's a real good question. Thanks for offering to test it; I think
that would be very helpful.


It seems to work just fine.  I didn't stress it too hard but I did put 
in one escape and a multi-byte character and check the various data types.


Attached is a test hack on pg_basebackup which produces this output:

START
FIELD "number", null 0
SCALAR TYPE 2: 123
FIELD "string", null 0
SCALAR TYPE 1: val  ue-丏
FIELD "bool", null 0
SCALAR TYPE 9: true
FIELD "null", null 1
SCALAR TYPE 11: null
END

I used the callbacks because that's the first method I found but it 
seems like json_lex() might be easier to use in practice.


I think it's an issue that the entire string must be passed to the lexer 
at once.  That will not be great for large manifests.  However, I don't 
think it will be all that hard to implement an optional "want more" 
callback in the lexer so JSON data can be fed in from the file in chunks.


So, that just leaves ereport() as the largest remaining issue?  I'll 
look at that today and Tuesday and see what I can work up.


Regards,
--
-David
da...@pgmasters.net
diff --git a/src/bin/pg_basebackup/pg_basebackup.c 
b/src/bin/pg_basebackup/pg_basebackup.c
index 238b671f7a..00b74118fb 100644
--- a/src/bin/pg_basebackup/pg_basebackup.c
+++ b/src/bin/pg_basebackup/pg_basebackup.c
@@ -41,6 +41,7 @@
 #include "receivelog.h"
 #include "replication/basebackup.h"
 #include "streamutil.h"
+#include "utils/jsonapi.h"
 
 #define ERRCODE_DATA_CORRUPTED "XX001"
 
@@ -2019,10 +2020,40 @@ BaseBackup(void)
pg_log_info("base backup completed");
 }
 
+void
+jsonOFieldAction(void *state, char *fname, bool isnull)
+{
+   fprintf(stderr, "FIELD \"%s\", null %d\n", fname, isnull); 
fflush(stderr);
+   pfree(fname);
+}
+
+void
+jsonScalarAction(void *state, char *token, JsonTokenType tokentype)
+{
+   fprintf(stderr, "SCALAR TYPE %u: %s\n", tokentype, token); 
fflush(stderr);
+   pfree(token);
+}
 
 int
 main(int argc, char **argv)
 {
+   if (true)
+   {
+   char json[] = "{\"number\": 123, \"string\": \"val\\tue-丏\", 
\"bool\": true, \"null\": null}";
+   JsonSemAction sem = {.semstate = NULL, .scalar = 
jsonScalarAction, .object_field_start = jsonOFieldAction};
+   JsonLexContext *lex;
+
+   fprintf(stderr, "START\n"); fflush(stderr);
+
+   lex = makeJsonLexContextCstringLen(json, strlen(json), true);
+
+   pg_parse_json(lex, );
+
+   fprintf(stderr, "END\n"); fflush(stderr);
+
+   exit(0);
+   }
+
static struct option long_options[] = {
{"help", no_argument, NULL, '?'},
{"version", no_argument, NULL, 'V'},

Re: Binary support for pgoutput plugin

[Dave Cramer]

On Mon, 2 Dec 2019 at 14:35, Dave Cramer  wrote:

> Rebased against head
>
> Dave Cramer
>
>
> On Sat, 30 Nov 2019 at 20:48, Michael Paquier  wrote:
>
>> Hi,
>>
>> On Mon, Nov 11, 2019 at 03:24:59PM -0500, Dave Cramer wrote:
>> > Attached,
>>
>> The latest patch set does not apply correctly.  Could you send a
>> rebase please?  I am moving the patch to next CF, waiting on author.
>> --
>> Michael
>>
>
Can I get someone to review this please ?
Dave Cramer

Re: [HACKERS] kqueue

[Rui DeSousa]

Thanks Thomas,

Just a quick update.

I just deployed this patch into a lower environment yesterday running FreeBSD 
12.1 and PostgreSQL 11.6.  I see a significant reduction is CPU/system load 
from load highs of 500+ down to the low 20’s.  System CPU time has been reduced 
to practically nothing.  

I’m working with our support vendor in testing the patch and will continue to 
let it burn in.  Hopefully, we can get the patched committed.  Thanks.

> On Dec 19, 2019, at 7:26 PM, Thomas Munro  wrote:
> 
> It's still my intention to get this committed eventually, but I got a
> bit frazzled by conflicting reports on several operating systems.  For
> FreeBSD, performance was improved in many cases, but there were also
> some regressions that seemed to be related to ongoing work in the
> kernel that seemed worth waiting for.  I don't have the details
> swapped into my brain right now, but there was something about a big
> kernel lock for Unix domain sockets which possibly explained some
> local pgbench problems, and there was also a problem relating to
> wakeup priority with some test parameters, which I'd need to go and
> dig up.  If you want to test this and let us know how you get on,
> that'd be great!  Here's a rebase against PostgreSQL's master branch,
> and since you mentioned PostgreSQL 11, here's a rebased version for
> REL_11_STABLE in case that's easier for you to test/build via ports or
> whatever and test with your production workload (eg on a throwaway
> copy of your production system).  You can see it's working by looking
> in top: instead of state "select" (which is how poll() is reported)
> you see "kqread", which on its own isn't exciting enough to get this
> committed :-)
> 

Re: Expose lock group leader pid in pg_stat_activity

[Julien Rouhaud]

On Thu, Jan 16, 2020 at 8:49 AM Michael Paquier  wrote:
>
> On Thu, Jan 16, 2020 at 04:27:27PM +0900, Michael Paquier wrote:
> > While looking at the code, I think that we could refactor things a bit
> > for raw_wait_event, wait_event_type and wait_event which has some
> > duplicated code for backend and auxiliary processes.  What about
> > filling in the wait event data after fetching the PGPROC entry, and
> > also fill in leader_pid for auxiliary processes.  This does not matter
> > now, perhaps it will never matter (or not), but that would make the
> > code much more consistent.
>
> And actually, the way you are looking at the leader's PID is visibly
> incorrect and inconsistent because the patch takes no shared LWLock on
> the leader using LockHashPartitionLockByProc() followed by
> LWLockAcquire(), no?  That's incorrect because it could be perfectly
> possible to crash with this code between the moment you check if
> lockGroupLeader is NULL and the moment you look at
> lockGroupLeader->pid if a process is being stopped in-between and
> removes itself from a lock group in ProcKill().  That's also
> inconsistent because it could be perfectly possible to finish with an
> incorrect view of the data while scanning for all the backend entries,
> like a leader set to NULL with workers pointing to the leader for
> example, or even workers marked incorrectly as NULL.  The second one
> may not be a problem, but the first one could be confusing.

Oh indeed.  But unless we hold some LWLock during the whole function
execution, we cannot guarantee a consistent view right?  And isn't it
already possible to e.g. see a parallel worker in pg_stat_activity
while all other queries are shown are idle, if you're unlucky enough?

Also, LockHashPartitionLockByProc requires the leader PGPROC, and
there's no guarantee that we'll see the leader before any of the
workers, so I'm unsure how to implement what you said.  Wouldn't it be
better to simply fetch the leader PGPROC after acquiring a shared
ProcArrayLock, and using that copy to display the pid, after checking
that we retrieved a non-null PGPROC?

Re: Expose lock group leader pid in pg_stat_activity

[Julien Rouhaud]

On Thu, Jan 16, 2020 at 8:28 AM Michael Paquier  wrote:
>
> On Fri, Dec 27, 2019 at 10:15:33AM +0100, Julien Rouhaud wrote:
> > I think that not using "parallel" to name this field will help to
> > avoid confusion if the lock group infrastructure is eventually used
> > for something else, but that's only true if indeed we explain what a
> > lock group is.
>
> As you already pointed out, src/backend/storage/lmgr/README includes a
> full description of this stuff under the section "Group Locking".  So
> I agree that the patch ought to document your new field in a much
> better way, without mentioning the term "group locking" that's even
> better to not confuse the reader because this term not present in the
> docs at all.
>
> > The leader_pid is NULL for processes not involved in parallel query.
> > When a process wants to cooperate with parallel workers, it becomes a
> > lock group leader, which means that this field will be valued to its
> > own pid. When a parallel worker starts up, this field will be valued
> > with the leader pid.
>
> The first sentence is good to have.  Now instead of "lock group
> leader", I think that we had better use "parallel group leader" as in
> other parts of the docs (see wait events for example).

Ok, I'll change this way.

>  Then we just
> need to say that if leader_pid has the same value as
> pg_stat_activity.pid, then we have a group leader.  If not, then it is
> a parallel worker process initially spawned by the leader whose PID is
> leader_pid (when executing Gather, Gather Merge, soon-to-be parallel
> vacuum or for a parallel btree build, but this does not need a mention
> in the docs).  There could be an argument as well to have leader_pid
> set to NULL for a leader, but that would be inconsistent with what
> the PGPROC entry reports for the backend.

It would also slightly complicate things to get the full set of
backends involved in a parallel query, while excluding the leader is
entirely trivial.

> While looking at the code, I think that we could refactor things a bit
> for raw_wait_event, wait_event_type and wait_event which has some
> duplicated code for backend and auxiliary processes.  What about
> filling in the wait event data after fetching the PGPROC entry, and
> also fill in leader_pid for auxiliary processes.  This does not matter
> now, perhaps it will never matter (or not), but that would make the
> code much more consistent.

Yeah, I didn't think that auxiliary would be involved any time soon
but I can include this refactoring.

Re: SlabCheck leaks memory into TopMemoryContext

[Tomas Vondra]

On Thu, Jan 16, 2020 at 01:41:39PM -0500, Tom Lane wrote:

Andres Freund  writes:

On 2020-01-16 18:01:53 +0100, Tomas Vondra wrote:

and it's only really used in debug builds anyway. So I'm not all that
woried about this wasting a couple extra kB of memory.



IDK, making memory usage look different makes optimizing it harder. Not
a hard rule, obviously, but ...


Well, if you're that excited about it, make a patch so we can see
how ugly it ends up being.



I think the question is how much memory would using globals actually
save, compared to including the bitmap in SlabContext.

The bitmap size depends on block/chunk size - I don't know what
parameters Andres uses for the additional contexts, but for the two
places already using Slab we have 8kB blocks with 80B and 240B chunks,
so ~102 and ~34 chunks in a block. So it's not a huge amount, and we
could easily reduce this to 1/8 by switching to a proper bitmap.


regards

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

Re: Complete data erasure

[Stephen Frost]

Greetings,

* asaba.takan...@fujitsu.com (asaba.takan...@fujitsu.com) wrote:
> This feature erases data area just before it is returned to the OS (“erase” 
> means that overwrite data area to hide its contents here) 
> because there is a risk that the data will be restored by attackers if it is 
> returned to the OS without being overwritten.
> The erase timing is when DROP, VACUUM, TRUNCATE, etc. are executed.

Looking at this fresh, I wanted to point out that I think Tom's right-
we aren't going to be able to reasonbly support this kind of data
erasure on a simple DROP TABLE or TRUNCATE.

> I want users to be able to customize the erasure method for their security 
> policies.

There's also this- but I think what it means is that we'd probably have
a top-level command that basically is "ERASE TABLE blah;" or similar
which doesn't operate during transaction commit but instead marks the
table as "to be erased" and then perhaps "erasure in progress" and then
"fully erased" (or maybe just back to 'normal' at that point).  Making
those updates will require the command to perform its own transaction
management which is why it can't be in a transaction itself but also
means that the data erasure process doesn't need to be done during
commit.

> My idea is adding a new parameter erase_command to postgresql.conf.

Yeah, I don't think that's really a sensible option or even approach.

> When erase_command is set, VACUUM does not truncate a file size to non-zero 
> because it's safer for users to return the entire file to the OS than to 
> return part of it.

There was discussion elsewhere about preventing VACUUM from doing a
truncate on a file because of the lock it requires and problems with
replicas..  I'm not sure where that ended up, but, in general, I don't
think this feature and VACUUM should really have anything to do with
each other except for the possible case that a user might be told to
configure their system to not allow VACUUM to truncate tables if they
care about this case.

As mentioned elsewhere, you do also have to consider that the sensitive
data will end up in the WAL and on replicas.  I don't believe that means
this feature is without use, but it means that users of this feature
will also need to understand and be able to address WAL and replicas
(along with backups and such too, of course).

Thanks,

Stephen


signature.asc
Description: PGP signature

Re: Errors when update a view with conditional-INSTEAD rules

[Dean Rasheed]

On Fri, 17 Jan 2020 at 06:14, Pengzhou Tang  wrote:
>
> I am wondering whether a simple auto-updatable view can have a conditional 
> update instead rule.

Well, the decision reached in [1] was that we wouldn't allow that. We
could decide to allow it now as a new feature enhancement, but it
wouldn't be a back-patchable bug-fix, and to be honest I wouldn't be
particularly excited about adding such a feature now. We already get
enough reports related to multiple rule actions behaving in
counter-intuitive ways that trip up users. I don't think we should be
enhancing the rule system, but rather encouraging users not to use it
and use triggers instead.

> The document also says that:
> "There is a catch if you try to use conditional rules for complex view 
> updates: there must be an unconditional
> INSTEAD rule for each action you wish to allow on the view" which makes me 
> think a simple view can have a
> conditional INSTEAD rule. And the document is explicitly changed in commit 
> a99c42f291421572aef2:
> -   There is a catch if you try to use conditional rules for view
> +  There is a catch if you try to use conditional rules for complex view
>
> Does that mean we should support conditional rules for a simple view?
>

No. I don't recall why that wording was changed in that commit, but I
think it's meant to be read as "complex updates on views" -- i.e., the
word "complex" refers to the complexity of the update logic, not the
complexity of the view. Nothing in that paragraph is related to
complex vs simple views, it's about complex sets of rules.

Regards,
Dean

[1] https://www.postgresql.org/message-id/25777.1352325888%40sss.pgh.pa.us

Re: SlabCheck leaks memory into TopMemoryContext

[Tomas Vondra]

On Thu, Jan 16, 2020 at 06:04:32PM +0100, Tomas Vondra wrote:

On Thu, Jan 16, 2020 at 11:43:34AM -0500, Tom Lane wrote:

Tomas Vondra  writes:

The attached fix should do the trick - it pre-allocates the space when
creating the context. There is a bit of complexity because we want to
allocate the space as part of the context header, but nothin too bad. We
might optimize it a bit by using a regular bitmap (instead of just an
array of bools), but I haven't done that.


Hmm ... so if this is an array of bools, why isn't it declared bool*
rather than char* ?  (Pre-existing ugliness, sure, but we might as
well fix it while we're here.  Especially since you used sizeof(bool)
in the space calculation.)



True. Will fix.


I agree that maxaligning the start point of the array is pointless.

I'd write "free chunks in a block" not "free chunks on a block",
the latter seems rather shaky English.  But that's getting picky.

LGTM otherwise.



OK. Barring objections I'll push and backpatch this later today.



I've pushed and backpatched this all the back back to 10.

regards

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

Re: TRUNCATE on foreign tables

[Kohei KaiGai]

Hi,

The v3 patch updated the points below:
- 2nd arg of ExecForeignTruncate was changed to DropBehavior, not bool
- ExecuteTruncateGuts() uses a local hash table to track a pair of server-id
  and list of the foreign tables managed by the server.
- Error message on truncate_check_rel() was revised as follows:
   "foreign data wrapper \"%s\" on behalf of the foreign table \"%s\"
does not support TRUNCATE"
- deparseTruncateSql() of postgres_fdw generates entire remote SQL as
like other commands.
- Document SGML was updated.

Best regards,

2020年1月16日(木) 14:40 Michael Paquier :
>
> On Wed, Jan 15, 2020 at 11:33:07PM +0900, Kohei KaiGai wrote:
> > 2020年1月15日(水) 17:11 Michael Paquier :
> >> I have done a quick read through the patch.  You have modified the
> >> patch to pass down to the callback a list of relation OIDs to execute
> >> one command for all, and there are tests for FKs so that coverage
> >> looks fine.
> >>
> >> Regression tests are failing with this patch:
> >>  -- TRUNCATE doesn't work on foreign tables, either directly or
> >>  recursively
> >>  TRUNCATE ft2;  -- ERROR
> >> -ERROR:  "ft2" is not a table
> >> +ERROR:  foreign-data wrapper "dummy" has no handler
> >> You visibly just need to update the output because no handlers are
> >> available for truncate in this case.
> >>
> > What error message is better in this case? It does not print "ft2" anywhere,
> > so user may not notice that "ft2" is the source of the error.
> > How about 'foreign table "ft2" does not support truncate' ?
>
> It sounds to me that this message is kind of right.  This FDW "dummy"
> does not have any FDW handler at all, so it complains about it.
> Having no support for TRUNCATE is something that may happen after
> that.  Actually, this error message from your patch used for a FDW
> which has a  handler but no TRUNCATE support could be reworked:
> +   if (!fdwroutine->ExecForeignTruncate)
> +   ereport(ERROR,
> +   (errcode(ERRCODE_WRONG_OBJECT_TYPE),
> +errmsg("\"%s\" is not a supported foreign table",
> +   relname)));
> Something like "Foreign-data wrapper \"%s\" does not support
> TRUNCATE"?
>
> >> + frels_list is a list of foreign tables that are
> >> + connected to a particular foreign server; thus, these foreign tables
> >> + should have identical foreign server ID
> >> The list is built by the backend code, so that has to be true.
> >>
> >> +   foreach (lc, frels_list)
> >> +   {
> >> +   Relationfrel = lfirst(lc);
> >> +   Oid frel_oid = RelationGetRelid(frel);
> >> +
> >> +   if (server_id == GetForeignServerIdByRelId(frel_oid))
> >> +   {
> >> +   frels_list = foreach_delete_current(frels_list, lc);
> >> +   curr_frels = lappend(curr_frels, frel);
> >> +   }
> >> +   }
> >> Wouldn't it be better to fill in a hash table for each server with a
> >> list of relations?
> >
> > It's just a matter of preference. A temporary hash-table with server-id
> > and list of foreign-tables is an idea. Let me try to revise.
>
> Thanks.  It would not matter much for relations without inheritance
> children, but if truncating a partition tree with many foreign tables
> using various FDWs that could matter performance-wise.
> --
> Michael



-- 
HeteroDB, Inc / The PG-Strom Project
KaiGai Kohei 

Re: Unnecessary delay in streaming replication due to replay lag

[Asim R P]

On Fri, Jan 17, 2020 at 11:08 AM Michael Paquier 
wrote:
>
> On Fri, Jan 17, 2020 at 09:34:05AM +0530, Asim R P wrote:
> >
> > 0001 - TAP test to demonstrate the problem.
>
> There is no real need for debug_replay_delay because we have already
> recovery_min_apply_delay, no?  That would count only after consistency
> has been reached, and only for COMMIT records, but your test would be
> enough with that.
>

Indeed, we didn't know about recovery_min_apply_delay.  Thank you for
the suggestion, the updated test is attached.

>
> > This is a POC, we are looking for early feedback on whether the
> > problem is worth solving and if it makes sense to solve if along this
> > route.
>
> You are not the first person interested in this problem, we have a
> patch registered in this CF to control the timing when a WAL receiver
> is started at recovery:
> https://commitfest.postgresql.org/26/1995/
>
https://www.postgresql.org/message-id/b271715f-f945-35b0-d1f5-c9de3e56f...@postgrespro.ru
>

Great to know about this patch and the discussion.  The test case and
the part that saves next start point in control file from our patch
can be combined with Konstantin's patch to solve this problem.  Let me
work on that.

> I am pretty sure that we should not change the default behavior to
> start the WAL receiver after replaying everything from the archives to
> avoid copying some WAL segments for nothing, so being able to use a
> GUC switch should be the way to go, and Konstantin's latest patch was
> using this approach.  Your patch 0002 adds visibly a third mode: start
> immediately on top of the two ones already proposed:
> - Start after replaying all WAL available locally and in the
> archives.
> - Start after reaching a consistent point.

Consistent point should be reached fairly quickly, in spite of large
replay lag.  Min recovery point is updated during XLOG flush and that
happens when a commit record is replayed.  Commits should occur
frequently in the WAL stream.  So I do not see much value in starting
WAL receiver immediately as compared to starting it after reaching a
consistent point.  Does that make sense?

That said, is there anything obviously wrong with starting WAL receiver
immediately, even before reaching consistent state?  A consequence is
that WAL receiver may overwrite a WAL segment while startup process is
reading and replaying WAL from it.  But that doesn't appear to be a
problem because the overwrite should happen with identical content as
before.

Asim


v1-0001-Test-that-replay-of-WAL-logs-on-standby-does-not-.patch
Description: Binary data


v1-0003-Start-WAL-receiver-when-it-is-found-not-running.patch
Description: Binary data


v1-0002-Start-WAL-receiver-before-startup-process-replays.patch
Description: Binary data

Re: [HACKERS] Block level parallel vacuum

[Mahendra Singh Thalor]

On Fri, 17 Jan 2020 at 14:47, Amit Kapila  wrote:
>
> On Fri, Jan 17, 2020 at 12:51 PM Dilip Kumar 
wrote:
> >
> > On Fri, Jan 17, 2020 at 11:39 AM Dilip Kumar 
wrote:
> > I have performed cost delay testing on the latest test(I have used
> > same script as attahced in [1] and [2].
> > vacuum_cost_delay = 10
> > vacuum_cost_limit = 2000
> >
> > Observation: As we have concluded earlier, the delay time is in sync
> > with the I/O performed by the worker
> > and the total delay (heap + index) is almost the same as the
> > non-parallel operation.
> >
>
> Thanks for doing this test again.  In the attached patch, I have
> addressed all the comments and modified a few comments.
>

Hi,
Below are some review comments for v50 patch.

1.
+LVShared
+LVSharedIndStats
+LVParallelState
 LWLock

I think, LVParallelState should come before LVSharedIndStats.

2.
+/*
+ * It is possible that parallel context is initialized with fewer
workers
+ * then the number of indexes that need a separate worker in the
current
+ * phase, so we need to consider it.  See
compute_parallel_vacuum_workers.
+ */

This comment is confusing me. I think, "then" should be replaced with
"than".

-- 
Thanks and Regards
Mahendra Singh Thalor
EnterpriseDB: http://www.enterprisedb.com

Re: PATCH: standby crashed when replay block which truncated in standby but failed to truncate in master node

[Fujii Masao]

On Fri, Jan 17, 2020 at 1:47 PM Michael Paquier  wrote:
>
> On Thu, Jan 16, 2020 at 11:17:36PM +0900, Fujii Masao wrote:
> > OK, I updated the patch that way.
> > Attached is the updated version of the patch.
>
> Thanks.  I have few tweaks to propose to the docs.
>
> +raise a PANIC-level error, aborting the recovery. Setting
> Instead of "PANIC-level error", I would just use "PANIC error", and

I have no strong opinion about this, but I used "PANIC-level error"
because the description for data_sync_retry has already used it.

> instead of "aborting the recovery" just "crashing the server".

PANIC implies server crash, so IMO "crashing the server" is
a bit redundant, and "aborting the recovery" is better because
"continue the recovery" is used later.

> +causes the system to ignore those WAL records
> WAL records are not ignored, but errors caused by incorrect page
> references in those WAL records are.  The current phrasing sounds like
> the WAL records are not applied.

So, what about

---
causes the system to ignore invalid page references in WAL records
(but still report a warning), and continue the recovery.
---

> Another thing that I just recalled.  Do you think that it would be
> better to mention that invalid page references can only be seen after
> reaching the consistent point during recovery?  The information given
> looks enough, but I was just wondering if that's worth documenting or
> not.

ISTM that this is not the information that users should understand...

Regards,

-- 
Fujii Masao

Re: Does 'instead of delete' trigger support modification of OLD

[Eugen Konkov]

Hello Bruce,

> Triggers are designed to check and modify input data, and since DELETE
> has no input data, it makes no sense.

Sorry,  I  am  still ambiguous. You say that DELETE has no input data,
but doc says that it has:

https://www.postgresql.org/docs/current/trigger-definition.html
For  a row-level trigger, the input data also includes ... the OLD row for ... 
DELETE triggers


Also  restricting  DELETE  to  change  the  returned  data  by  DELETE
RETURNING seems as incomplete.

For example if triggers implement some compression.
 -- insert into field Z value
 -- compress and actually store Zx5 into field
 -- Delete this insert row
 -- So user should get back that the value Z was deleted and not Zx5.
 Correct?

 but currently user will see Zx5, because next code:

 OLD.value =  uncompress( OLD.value );

 does not effect RETURNING =(

-- 
Best regards,
Eugen Konkov

Re: [HACKERS] Block level parallel vacuum

[Amit Kapila]

On Fri, Jan 17, 2020 at 12:51 PM Dilip Kumar  wrote:
>
> On Fri, Jan 17, 2020 at 11:39 AM Dilip Kumar  wrote:
> I have performed cost delay testing on the latest test(I have used
> same script as attahced in [1] and [2].
> vacuum_cost_delay = 10
> vacuum_cost_limit = 2000
>
> Observation: As we have concluded earlier, the delay time is in sync
> with the I/O performed by the worker
> and the total delay (heap + index) is almost the same as the
> non-parallel operation.
>

Thanks for doing this test again.  In the attached patch, I have
addressed all the comments and modified a few comments.

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


v50-0001-Allow-vacuum-command-to-process-indexes-in-parallel.patch
Description: Binary data

Re: Incremental View Maintenance: ERROR: out of shared memory

[Yugo NAGATA]

On Sun, 29 Dec 2019 12:27:13 -0500
Tom Lane  wrote:

> Tatsuo Ishii  writes:
> >> here is an unexpected error found while testing IVM v11 patches
> >> ...
> >> ERROR:  out of shared memory
> 
> > I think we could avoid such an error in IVM by reusing a temp table in
> > a session or a transaction.
> 
> I'm more than a little bit astonished that this proposed patch is
> creating temp tables at all.  ISTM that that implies that it's
> being implemented at the wrong level of abstraction, and it will be
> full of security problems, as well as performance problems above
> and beyond the one described here.

We realized that there is also other problems in using temp tables
as pointed out in another thread. So, we are now working on rewrite
our patch not to use temp tables.

Regards,
Yugo Nagata


-- 
Yugo NAGATA 

RE: Complete data erasure

[asaba.takan...@fujitsu.com]

Hello, Horiguchi-san

Thank you for comment.

At Wed, 15 Jan 2020 03:46 +, "Kyotaro Horiguchi " 
wrote in
> shred(1) or wipe(1) doesn't seem to contribute to the objective on
> journaled or copy-on-write file systems. I'm not sure, but maybe the
> same can be true for read-modify-write devices like SSD. I'm not sure
> about SDelete, but anyway replacing unlink() with something like
> 'system("shred")' leads to siginificant performance degradation.
> 
> man 1 wipe says (https://linux.die.net/man/1/wipe) : (shred has a
> similar note.)
> 
> > NOTE ABOUT JOURNALING FILESYSTEMS AND SOME RECOMMENDATIONS
> (JUNE 2004)
> > Journaling filesystems (such as Ext3 or ReiserFS) are now being used
> > by default by most Linux distributions. No secure deletion program
> > that does filesystem-level calls can sanitize files on such
> > filesystems, because sensitive data and metadata can be written to the
> > journal, which cannot be readily accessed. Per-file secure deletion is
> > better implemented in the operating system.

shred can be used in certain modes of journaled file systems.
How about telling users that they must set the certain mode
if they set shred for erase_command in journaled file systems?
man 1 shred goes on like this:

> In  the  case of ext3 file systems, the above disclaimer applies (and shred 
> is thus
> of limited effectiveness) only in data=journal mode, which journals  file  
> data  in
> addition  to  just metadata.  In both the data=ordered (default) and 
> data=writeback
> modes, shred works as usual.  Ext3 journaling modes can be changed  by  
> adding  the
> data=something  option  to  the  mount  options for a particular file system 
> in the
> /etc/fstab file, as documented in the mount man page (man mount).

As shown above, shred works as usual in both the data=ordered (default) and 
data=writeback modes.
I think data=journal mode is not used in many cases because it degrades 
performance.
Therefore, I think it is enough to indicate that shred cannot be used in 
data=journal mode.

Regards,

--
Takanori Asaba

Re: Implementing Incremental View Maintenance

[Yugo NAGATA]

On Thu, 16 Jan 2020 18:50:40 +0900
nuko yokohama  wrote:

> Error occurs when updating user-defined type columns.
> 
> Create an INCREMENTAL MATERIALIZED VIEW by specifying a query that includes
> user-defined type columns.
> After the view is created, an error occurs when inserting into the view
> source table (including the user-defined type column).
> ```
> ERROR:  operator does not exist

Thank you for your reporting. I think this error occurs because 
pg_catalog.= is used also for user-defined types. I will fix this.

Regards,
Yugo Nagata

> ```
> 
> An execution example is shown below.
> 
> ```
> [ec2-user@ip-10-0-1-10 ivm]$ psql testdb -a -f extension-insert.sql
> --
> -- pg_fraction: https://github.com/nuko-yokohama/pg_fraction
> --
> DROP EXTENSION IF EXISTS pg_fraction CASCADE;
> psql:extension-insert.sql:4: NOTICE:  drop cascades to column data of table
> foo
> DROP EXTENSION
> DROP TABLE IF EXISTS foo CASCADE;
> DROP TABLE
> CREATE EXTENSION IF NOT EXISTS pg_fraction;
> CREATE EXTENSION
> \dx
>List of installed extensions
> Name | Version |   Schema   | Description
> -+-++--
>  pg_fraction | 1.0 | public | fraction data type
>  plpgsql | 1.0 | pg_catalog | PL/pgSQL procedural language
> (2 rows)
> 
> \dT+ fraction
> List of data types
>  Schema |   Name   | Internal name | Size | Elements |  Owner   | Access
> privileges | Description
> +--+---+--+--+--+---+-
>  public | fraction | fraction  | 16   |  | postgres |
> |
> (1 row)
> 
> CREATE TABLE foo (id int, data fraction);
> CREATE TABLE
> INSERT INTO foo (id, data) VALUES (1,'2/3'),(2,'1/3'),(3,'1/2');
> INSERT 0 3
> SELECT id, data FROM foo WHERE data >= '1/2';
>  id | data
> +--
>   1 | 2/3
>   3 | 1/2
> (2 rows)
> 
> CREATE INCREMENTAL MATERIALIZED VIEW foo_imv AS SELECT id, data FROM foo
> WHERE data >= '1/2';
> SELECT 2
> TABLE foo_imv;
>  id | data
> +--
>   1 | 2/3
>   3 | 1/2
> (2 rows)
> 
> INSERT INTO foo (id, data) VALUES (4,'2/3'),(5,'2/5'),(6,'3/6'); -- error
> psql:extension-insert.sql:17: ERROR:  operator does not exist: fraction
> pg_catalog.= fraction
> LINE 1: ...(mv.id IS NULL AND diff.id IS NULL)) AND (mv.data OPERATOR(p...
>  ^
> HINT:  No operator matches the given name and argument types. You might
> need to add explicit type casts.
> QUERY:  WITH updt AS (UPDATE public.foo_imv AS mv SET __ivm_count__ =
> mv.__ivm_count__ OPERATOR(pg_catalog.+) diff.__ivm_count__  FROM
> pg_temp_3.pg_temp_73900 AS diff WHERE (mv.id OPERATOR(pg_catalog.=) diff.id
> OR (mv.id IS NULL AND diff.id IS NULL)) AND (mv.data OPERATOR(pg_catalog.=)
> diff.data OR (mv.data IS NULL AND diff.data IS NULL)) RETURNING mv.id,
> mv.data) INSERT INTO public.foo_imv SELECT * FROM pg_temp_3.pg_temp_73900
> AS diff WHERE NOT EXISTS (SELECT 1 FROM updt AS mv WHERE (mv.id
> OPERATOR(pg_catalog.=) diff.id OR (mv.id IS NULL AND diff.id IS NULL)) AND
> (mv.data OPERATOR(pg_catalog.=) diff.data OR (mv.data IS NULL AND diff.data
> IS NULL)));
> TABLE foo;
>  id | data
> +--
>   1 | 2/3
>   2 | 1/3
>   3 | 1/2
> (3 rows)
> 
> TABLE foo_imv;
>  id | data
> +--
>   1 | 2/3
>   3 | 1/2
> (2 rows)
> 
> DROP MATERIALIZED VIEW foo_imv;
> DROP MATERIALIZED VIEW
> INSERT INTO foo (id, data) VALUES (4,'2/3'),(5,'2/5'),(6,'3/6');
> INSERT 0 3
> TABLE foo;
>  id | data
> +--
>   1 | 2/3
>   2 | 1/3
>   3 | 1/2
>   4 | 2/3
>   5 | 2/5
>   6 | 1/2
> (6 rows)
> 
> ```
> 
> Best regards.
> 
> 2018年12月27日(木) 21:57 Yugo Nagata :
> 
> > Hi,
> >
> > I would like to implement Incremental View Maintenance (IVM) on
> > PostgreSQL.
> > IVM is a technique to maintain materialized views which computes and
> > applies
> > only the incremental changes to the materialized views rather than
> > recomputate the contents as the current REFRESH command does.
> >
> > I had a presentation on our PoC implementation of IVM at PGConf.eu 2018
> > [1].
> > Our implementation uses row OIDs to compute deltas for materialized
> > views.
> > The basic idea is that if we have information about which rows in base
> > tables
> > are contributing to generate a certain row in a matview then we can
> > identify
> > the affected rows when a base table is updated. This is based on an idea of
> > Dr. Masunaga [2] who is a member of our group and inspired from ID-based
> > approach[3].
> >
> > In our implementation, the mapping of the row OIDs of the materialized view
> > and the base tables are stored in "OID map". When a base relation is
> > modified,
> > AFTER trigger is executed and the delta is recorded in delta tables using
> > the transition table feature. The accual udpate of the matview is triggerd
> > by REFRESH command with INCREMENTALLY option.
> >
> > However, 

Re: FETCH FIRST clause PERCENT option

[Vik Fearing]

On 17/01/2020 07:20, Kyotaro Horiguchi wrote:
> Thank you for the notification, Tomas.
>
> At Thu, 16 Jan 2020 22:33:58 +0100, Tomas Vondra 
>  wrote in 
>> This patch is marked as RFC since September. Since then there was no
>> discussion on this thread, but Andrew proposed an alternative approach
>> based on window functions in a separate thread [1] (both for this and
>> for the WITH TIES case).
>>
>> I'll set this patch back to "needs review" - at this point we need to
>> decide which of the approaches is the right one.
> Even consdiering that we have only two usage , WITH TIES and PERCENT,
> of the mechanism for now, I like [1] for the generic nature,
> simpleness and smaller invasiveness to executor. The fact that cursor
> needs materialize to allow backward scan would not be a big problem.
> It seems that PERCENT will be easily implemented on that.
>
> However, isn't there a possibility that we allow FETCH FIRST x PERCENT
> WITH TIES, though I'm not sure what SQL spec tells about that?  I
> don't come up right now how to do that using window functions..


PERCENT and WITH TIES can play together, per spec.

-- 

Vik Fearing

Re: Implementing Incremental View Maintenance

[Yugo NAGATA]

On Thu, 16 Jan 2020 12:59:11 +0900
nuko yokohama  wrote:

> Aggregate operation of user-defined type cannot be specified
> (commit e150d964df7e3aeb768e4bae35d15764f8abd284)
> 
> A SELECT statement using the MIN() and MAX() functions can be executed on a
> user-defined type column that implements the aggregate functions MIN () and
> MAX ().
> However, if the same SELECT statement is specified in the AS clause of
> CREATE INCREMENTAL MATERIALIZED VIEW, the following error will occur.
> 
> ```
> SELECT MIN(data) data_min, MAX(data) data_max FROM foo;
>  data_min | data_max
> --+--
>  1/3  | 2/3
> (1 row)
> 
> CREATE INCREMENTAL MATERIALIZED VIEW foo_min_imv AS SELECT MIN(data)
> data_min FROM foo;
> psql:extension-agg.sql:14: ERROR:  aggregate function min is not supported
> CREATE INCREMENTAL MATERIALIZED VIEW foo_max_imv AS SELECT MAX(data)
> data_max FROM foo;
> psql:extension-agg.sql:15: ERROR:  aggregate function max is not supported
> ```
> 
> Does query including user-defined type aggregate operation not supported by
> INCREMENTAL MATERIALIZED VIEW?

The current implementation supports only built-in aggregate functions, so
user-defined aggregates are not supported, although it is allowed before.
This is because we can not know how user-defined aggregates behave and if
it can work safely with IVM. Min/Max on your fraction type may work well, 
but it is possible that some user-defined aggregate functions named min
or max behave in totally different way than we expected.

In future, maybe it is possible support user-defined aggregates are supported
by extending pg_aggregate and adding support functions for IVM, but there is
not still a concrete plan for now. 

BTW, the following error message doesn't look good because built-in min is
supported, so I will improve it.

 ERROR:  aggregate function min is not supported

Regards,
Yugo Nagata

> 
> An execution example is shown below.
> 
> ```
> [ec2-user@ip-10-0-1-10 ivm]$ cat extension-agg.sql
> --
> -- pg_fraction: https://github.com/nuko-yokohama/pg_fraction
> --
> DROP EXTENSION IF EXISTS pg_fraction CASCADE;
> DROP TABLE IF EXISTS foo CASCADE;
> 
> CREATE EXTENSION IF NOT EXISTS pg_fraction;
> \dx
> \dT+ fraction
> 
> CREATE TABLE foo (id int, data fraction);
> INSERT INTO foo (id, data) VALUES (1,'2/3'),(2,'1/3'),(3,'1/2');
> SELECT MIN(data) data_min, MAX(data) data_max FROM foo;
> CREATE INCREMENTAL MATERIALIZED VIEW foo_min_imv AS SELECT MIN(data)
> data_min FROM foo;
> CREATE INCREMENTAL MATERIALIZED VIEW foo_max_imv AS SELECT MAX(data)
> data_max FROM foo;
> 
> SELECT MIN(id) id_min, MAX(id) id_max FROM foo;
> CREATE INCREMENTAL MATERIALIZED VIEW foo_id_imv AS SELECT MIN(id) id_min,
> MAX(id) id_max FROM foo;
> ```
> 
> Best regards.
> 
> 2018年12月27日(木) 21:57 Yugo Nagata :
> 
> > Hi,
> >
> > I would like to implement Incremental View Maintenance (IVM) on
> > PostgreSQL.
> > IVM is a technique to maintain materialized views which computes and
> > applies
> > only the incremental changes to the materialized views rather than
> > recomputate the contents as the current REFRESH command does.
> >
> > I had a presentation on our PoC implementation of IVM at PGConf.eu 2018
> > [1].
> > Our implementation uses row OIDs to compute deltas for materialized
> > views.
> > The basic idea is that if we have information about which rows in base
> > tables
> > are contributing to generate a certain row in a matview then we can
> > identify
> > the affected rows when a base table is updated. This is based on an idea of
> > Dr. Masunaga [2] who is a member of our group and inspired from ID-based
> > approach[3].
> >
> > In our implementation, the mapping of the row OIDs of the materialized view
> > and the base tables are stored in "OID map". When a base relation is
> > modified,
> > AFTER trigger is executed and the delta is recorded in delta tables using
> > the transition table feature. The accual udpate of the matview is triggerd
> > by REFRESH command with INCREMENTALLY option.
> >
> > However, we realize problems of our implementation. First, WITH OIDS will
> > be removed since PG12, so OIDs are no longer available. Besides this, it
> > would
> > be hard to implement this since it needs many changes of executor nodes to
> > collect base tables's OIDs during execuing a query. Also, the cost of
> > maintaining
> > OID map would be high.
> >
> > For these reasons, we started to think to implement IVM without relying on
> > OIDs
> > and made a bit more surveys.
> >
> > We also looked at Kevin Grittner's discussion [4] on incremental matview
> > maintenance.  In this discussion, Kevin proposed to use counting algorithm
> > [5]
> > to handle projection views (using DISTNICT) properly. This algorithm need
> > an
> > additional system column, count_t, in materialized views and delta tables
> > of
> > base tables.
> >
> > However, the discussion about IVM is now stoped, so we would like to
> > restart and
> > progress this.
> >
> >
> > Through our