Re: [HACKERS] Row Level Security Bug ?

[Joe Conway]

On 11/12/2017 10:17 AM, Andrea Adami wrote:
> if i do:
> 
> SET ROLE 'manage...@scuola-1.it '

[SELECT from table]

> i see only one row (as expected)
> 
> but when i do:

[SELECT from VIEWs]

> I see all the rows always
> 
> this way i lack all the row level security i defined
> 
> is this either a bug or it's made by design ?
> if it's made by design why ?
> Is there  a way to write view that respect the row level security ?
> For my point of view is a nonsense make a row level security that
> doesn't work with the view.

See:
https://www.postgresql.org/docs/10/static/sql-createview.html
In particular: "Access to tables referenced in the view is determined by
permissions of the view owner."

And:
https://www.postgresql.org/docs/10/static/ddl-rowsecurity.html
"Superusers and roles with the BYPASSRLS attribute always bypass the row
security system when accessing a table. Table owners normally bypass row
security as well, though a table owner can choose to be subject to row
security with ALTER TABLE ... FORCE ROW LEVEL SECURITY."

HTH,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] libpq connection strings: control over the cipher suites?

[Joe Conway]

On 11/09/2017 03:17 PM, Michael Paquier wrote:
> On Fri, Nov 10, 2017 at 2:53 AM, Joe Conway <m...@joeconway.com> wrote:
>> On 11/09/2017 03:27 AM, Graham Leggett wrote:
>>> Is there a parameter or mechanism for setting the required ssl cipher list 
>>> from the client side?
>>
>> I don't believe so. That is controlled by ssl_ciphers, which requires a
>> restart in order to change.
>>
>> https://www.postgresql.org/docs/10/static/runtime-config-connection.html#GUC-SSL-CIPHERS
> 
> Since commit de41869 present in v10, SSL parameters can be reloaded.

Oh, cool, I must have missed that -- thanks!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] libpq connection strings: control over the cipher suites?

[Joe Conway]

On 11/09/2017 03:27 AM, Graham Leggett wrote:
> Is there a parameter or mechanism for setting the required ssl cipher list 
> from the client side?

I don't believe so. That is controlled by ssl_ciphers, which requires a
restart in order to change.

https://www.postgresql.org/docs/10/static/runtime-config-connection.html#GUC-SSL-CIPHERS

select name,setting,context from pg_settings where name like '%ssl%';
   name| setting  |  context
---+--+
 ssl   | off  | postmaster
 ssl_ca_file   |  | postmaster
 ssl_cert_file | server.crt   | postmaster
 ssl_ciphers   | HIGH:MEDIUM:+3DES:!aNULL | postmaster
 ssl_crl_file  |  | postmaster
 ssl_ecdh_curve| prime256v1   | postmaster
 ssl_key_file  | server.key   | postmaster
 ssl_prefer_server_ciphers | on   | postmaster
(8 rows)

HTH,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] pg_regress help output

[Joe Conway]

On 10/14/2017 02:04 PM, Peter Eisentraut wrote:
> On 10/10/17 22:31, Joe Conway wrote:
>>> Also, why is the patch apparently changing whitespace in all the help
>>> lines?  Seems like that will create a lot of make-work for translators.
>> I debated with myself about that.
> 
> Well, there are no translations of pg_regress, so please change the
> whitespace to make it look best.

Committed that way.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] pg_control_recovery() return value when not in recovery

[Joe Conway]

On 09/17/2017 11:29 PM, Andres Freund wrote:
> On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:
>> On 18 September 2017 at 05:50, Andres Freund  wrote:
>> > Hi,
>> >
>> > Just noticed that we're returning the underlying values for
>> > pg_control_recovery() without any checks:
>> > postgres[14388][1]=# SELECT * FROM pg_control_recovery();
>> > ┌──┬───┬──┬┬───┐
>> > │ min_recovery_end_lsn │ min_recovery_end_timeline │ backup_start_lsn │ 
>> > backup_end_lsn │ end_of_backup_record_required │
>> > ├──┼───┼──┼┼───┤
>> > │ 0/0  │ 0 │ 0/0  │ 
>> > 0/0│ f │
>> > └──┴───┴──┴┴───┘
>> > (1 row)
>> 
>> Yes, that would have made sense for these to be NULL
> 
> Yea, that's what I think was well.  Joe, IIRC that's your code, do you
> agree as well?

Sorry for the slow response, but thinking back on this now, the idea of
these functions, in my mind at least, was to provide as close to the
same output as possible to what pg_controldata outputs. So:

# pg_controldata
...
Minimum recovery ending location: 0/0
Min recovery ending loc's timeline:   0
Backup start location:0/0
Backup end location:  0/0
End-of-backup record required:no
...

So if we make a change here, do we also change pg_controldata?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] pg_regress help output

[Joe Conway]

On 10/10/2017 07:06 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> I have been annoyed at least twice now by the lack of pg_regress command
>> line help output for the "--bindir=" option. In passing I noted
>> that there was no output for "--help" or "--version" options either.
> 
>> Any objections to the attached?
> 
> +1 for documenting it, but the phrasing seems a bit awkward:
> 
> ! printf(_("  --bindir=BINPATH  use BINPATH for programs that 
> are run;\n"));
> ! printf(_("if BINPATH empty, use PATH 
> from the environment\n"));
> 
> Maybe just "if empty, use PATH ..." ?

Ok, so like this?

8<--
--bindir=BINPATH   use BINPATH for programs that are run;\n"));
   if empty, use PATH from the environment\n"));
8<--

> Also, why is the patch apparently changing whitespace in all the help
> lines?  Seems like that will create a lot of make-work for translators.

I debated with myself about that.

In other cases, e.g. initdb or psql, where we mix short+long options and
long-only options, we indent the long-only options in the output to
match up with the long-options of the mixed lines (whew, hopefully that
is clear).

Previously we were not showing mixed short+long options for pg_regress
at all, and hence only indenting the long-only options minimally. But
the addition of -h and -V (again consistent with other programs we
ship), it made sense to be consistent in the way we indent.

But I am fine with leaving the original lines output the way they were
if preferred.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] pg_regress help output

[Joe Conway]

I have been annoyed at least twice now by the lack of pg_regress command
line help output for the "--bindir=" option. In passing I noted
that there was no output for "--help" or "--version" options either.

Any objections to the attached? It could be argued that it ought to be
back-patched too, but I won't bother unless someone cares enough to
request it.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/test/regress/pg_regress.c b/src/test/regress/pg_regress.c
index 7c628df..f3dcc1f 100644
*** a/src/test/regress/pg_regress.c
--- b/src/test/regress/pg_regress.c
*** help(void)
*** 2004,2040 
  	printf(_("Usage:\n  %s [OPTION]... [EXTRA-TEST]...\n"), progname);
  	printf(_("\n"));
  	printf(_("Options:\n"));
! 	printf(_("  --config-auth=DATADIR update authentication settings for DATADIR\n"));
! 	printf(_("  --create-role=ROLEcreate the specified role before testing\n"));
! 	printf(_("  --dbname=DB   use database DB (default \"regression\")\n"));
! 	printf(_("  --debug   turn on debug mode in programs that are run\n"));
! 	printf(_("  --dlpath=DIR  look for dynamic libraries in DIR\n"));
! 	printf(_("  --encoding=ENCODING   use ENCODING as the encoding\n"));
! 	printf(_("  --inputdir=DIRtake input files from DIR (default \".\")\n"));
! 	printf(_("  --launcher=CMDuse CMD as launcher of psql\n"));
! 	printf(_("  --load-extension=EXT  load the named extension before running the\n"));
! 	printf(_("tests; can appear multiple times\n"));
! 	printf(_("  --load-language=LANG  load the named language before running the\n"));
! 	printf(_("tests; can appear multiple times\n"));
! 	printf(_("  --max-connections=N   maximum number of concurrent connections\n"));
! 	printf(_("(default is 0, meaning unlimited)\n"));
! 	printf(_("  --max-concurrent-tests=N  maximum number of concurrent tests in schedule\n"));
! 	printf(_("(default is 0, meaning unlimited)\n"));
! 	printf(_("  --outputdir=DIR   place output files in DIR (default \".\")\n"));
! 	printf(_("  --schedule=FILE   use test ordering schedule from FILE\n"));
! 	printf(_("(can be used multiple times to concatenate)\n"));
! 	printf(_("  --temp-instance=DIR   create a temporary instance in DIR\n"));
! 	printf(_("  --use-existinguse an existing installation\n"));
  	printf(_("\n"));
  	printf(_("Options for \"temp-instance\" mode:\n"));
! 	printf(_("  --no-locale   use C locale\n"));
! 	printf(_("  --port=PORT   start postmaster on PORT\n"));
! 	printf(_("  --temp-config=FILEappend contents of FILE to temporary config\n"));
  	printf(_("\n"));
  	printf(_("Options for using an existing installation:\n"));
! 	printf(_("  --host=HOST   use postmaster running on HOST\n"));
! 	printf(_("  --port=PORT   use postmaster running at PORT\n"));
! 	printf(_("  --user=USER   connect as USER\n"));
  	printf(_("\n"));
  	printf(_("The exit status is 0 if all tests passed, 1 if some tests failed, and 2\n"));
  	printf(_("if the tests could not be run for some reason.\n"));
--- 2004,2044 
  	printf(_("Usage:\n  %s [OPTION]... [EXTRA-TEST]...\n"), progname);
  	printf(_("\n"));
  	printf(_("Options:\n"));
! 	printf(_("  --bindir=BINPATH  use BINPATH for programs that are run;\n"));
! 	printf(_("if BINPATH empty, use PATH from the environment\n"));
! 	printf(_("  --config-auth=DATADIR update authentication settings for DATADIR\n"));
! 	printf(_("  --create-role=ROLEcreate the specified role before testing\n"));
! 	printf(_("  --dbname=DB   use database DB (default \"regression\")\n"));
! 	printf(_("  --debug   turn on debug mode in programs that are run\n"));
! 	printf(_("  --dlpath=DIR  look for dynamic libraries in DIR\n"));
! 	printf(_("  --encoding=ENCODING   use ENCODING as the encoding\n"));
! 	printf(_("  -h, --helpshow this help, then exit\n"));
! 	printf(_("  --inputdir=DIRtake input files from DIR (default \".\")\n"));
! 	printf(_("  --launcher=CMDuse CMD as launcher of psql\n"));
! 	printf(_("  --load-extension=EXT  load the named extension before running the\n"));
! 	printf(_("tests; can appear multiple times\n"));
! 	printf(_("  --load-language=LANG  load the named language before running the\n"));
! 	printf(_("tests; can appear multiple times\n"));
! 	printf(_("  --max-connections=N   maximum number of concurrent connections\n"));
! 	printf(_(" 

Re: [HACKERS] search path security issue?

[Joe Conway]

On 10/06/2017 12:52 AM, Magnus Hagander wrote:
> It would be a nice feature to have in general, like a "basic guc
> permissions" thing. At least allowing a superuser to prevent exactly
> this. You could argue the same thing for example for memory parameters
> and such. We have no permissions at all when it comes to userset gucs
> today -- and of course, if something should be added about this, it
> should be done in a way that works for all the userset variables, not
> just search_path. 

+1

I have wished for exactly that more than once before.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] pg_control_recovery() return value when not in recovery

[Joe Conway]

Sorry for the top post. Sounds reasonable to me. Cannot look closely until 
Tuesday or so.

Joe


On September 17, 2017 11:29:32 PM PDT, Andres Freund  wrote:
>On 2017-09-18 07:24:43 +0100, Simon Riggs wrote:
>> On 18 September 2017 at 05:50, Andres Freund 
>wrote:
>> > Hi,
>> >
>> > Just noticed that we're returning the underlying values for
>> > pg_control_recovery() without any checks:
>> > postgres[14388][1]=# SELECT * FROM pg_control_recovery();
>> >
>┌──┬───┬──┬┬───┐
>> > │ min_recovery_end_lsn │ min_recovery_end_timeline │
>backup_start_lsn │ backup_end_lsn │ end_of_backup_record_required │
>> >
>├──┼───┼──┼┼───┤
>> > │ 0/0  │ 0 │ 0/0   
>  │ 0/0│ f │
>> >
>└──┴───┴──┴┴───┘
>> > (1 row)
>> 
>> Yes, that would have made sense for these to be NULL
>
>Yea, that's what I think was well.  Joe, IIRC that's your code, do you
>agree as well?
>
>
>> > postgres[14388][1]=# SELECT pg_is_in_recovery();
>> > ┌───┐
>> > │ pg_is_in_recovery │
>> > ├───┤
>> > │ f │
>> > └───┘
>> > (1 row)
>> 
>> But not this, since it is a boolean and the answer is known.
>
>Oh, that was just for reference, to show that the cluster isn't in
>recovery...
>
>
>- Andres

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

Re: odd behavior/possible bug (Was: Re: [HACKERS] PG10 partitioning - odd behavior/possible bug)

[Joe Conway]

On 09/03/2017 03:34 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> Notice that tsr is not empty at all on the first loop, but on the second
>> loop it is empty every second time the trigger fires.
> 
> I think the issue is that now() isn't changing within the transaction,
> so when you construct "tstzrange(lower(OLD.tr), now(), '[)')" using an
> old row whose "lower(OLD.tr)" is already "now()", you get an empty range.
> Probably using '[]' bounds would avoid the oddness.

Hmmm, good point. Sorry for the noise.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

odd behavior/possible bug (Was: Re: [HACKERS] PG10 partitioning - odd behavior/possible bug)

[Joe Conway]

On 09/03/2017 02:28 PM, Joe Conway wrote:
> I was playing around with partitioning and found an oddity that is best
> described with the following reasonably minimal test case:



> Notice that in the first loop iteration tsr is calculated from OLD.tr
> correctly. But in the second loop iteration it is not, and therefore no
> partition can be found for the insert.
> 
> I have not dug too deeply into this yet, but was wondering if this
> behavior is sane/expected for some reason I am missing?

This does not require partitioning to reproduce -- sorry for the false
accusations ;-)

8<---
CREATE TABLE timetravel
(
  id int8,
  f1 text not null,
  tr tstzrange not null default tstzrange(now(), 'infinity', '[]')
);

CREATE OR REPLACE FUNCTION modify_timetravel()
RETURNS TRIGGER AS $$
  DECLARE
tsr tstzrange;
  BEGIN
RAISE NOTICE 'OLD.tr = %', OLD.tr;

tsr := tstzrange(lower(OLD.tr), now(), '[)');
RAISE NOTICE 'tsr = %', tsr;

OLD.tr = tsr;
INSERT INTO timetravel VALUES (OLD.*);
IF (TG_OP = 'UPDATE') THEN
  tsr := tstzrange(now(), 'infinity', '[]');
  RAISE NOTICE 'NEW.tr = %', tsr;
  NEW.tr = tsr;
  RETURN NEW;
ELSIF (TG_OP = 'DELETE') THEN
  RETURN OLD;
END IF;
  END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER timetravel_audit BEFORE DELETE OR UPDATE
ON timetravel FOR EACH ROW EXECUTE PROCEDURE modify_timetravel();

INSERT INTO timetravel(id, f1)
SELECT g.i, 'row-' || g.i::text
FROM generate_series(1,10) AS g(i);

DO $$
  DECLARE
i int;
  BEGIN
FOR i IN 1..2 LOOP
  RAISE NOTICE 'loop = %', i;
  UPDATE timetravel SET f1 = f1 || '-r' || i where id < 4;
END LOOP;
  END
$$;
NOTICE:  loop = 1
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07",infinity]
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  loop = 2
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  tsr = ["2017-09-03 15:17:31.598734-07","2017-09-03
15:17:31.608018-07")
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  OLD.tr = ["2017-09-03 15:17:31.608018-07",infinity]
NOTICE:  tsr = empty
NOTICE:  NEW.tr = ["2017-09-03 15:17:31.608018-07",infinity]
DO
8<---

Notice that tsr is not empty at all on the first loop, but on the second
loop it is empty every second time the trigger fires.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] PG10 partitioning - odd behavior/possible bug

[Joe Conway]

I was playing around with partitioning and found an oddity that is best
described with the following reasonably minimal test case:

8<-
CREATE TABLE timetravel
(
  id int8,
  f1 text not null,
  tr tstzrange not null default tstzrange(now(), 'infinity', '[]')
) PARTITION BY RANGE (upper(tr));

CREATE TABLE timetravel_current PARTITION OF timetravel
(
  primary key (id, tr) DEFERRABLE
) FOR VALUES FROM ('infinity') TO (MAXVALUE);
CREATE INDEX timetravel_current_tr_idx ON timetravel_current USING GIST
(tr);

CREATE TABLE timetravel_history PARTITION OF timetravel
(
  primary key (id, tr) DEFERRABLE
) FOR VALUES FROM (MINVALUE) TO ('infinity');
CREATE INDEX timetravel_history_tr_idx ON timetravel_history USING GIST
(tr);

CREATE OR REPLACE FUNCTION modify_timetravel()
RETURNS TRIGGER AS $$
  DECLARE
tsr tstzrange;
  BEGIN
RAISE NOTICE 'OLD.tr = %', OLD.tr;

tsr := tstzrange(lower(OLD.tr), now(), '[)');
RAISE NOTICE 'tsr = %', tsr;

OLD.tr = tsr;
INSERT INTO timetravel VALUES (OLD.*);
IF (TG_OP = 'UPDATE') THEN
  tsr := tstzrange(now(), 'infinity', '[]');
  RAISE NOTICE 'NEW.tr = %', tsr;
  NEW.tr = tsr;
  RETURN NEW;
ELSIF (TG_OP = 'DELETE') THEN
  RETURN OLD;
END IF;
  END;
$$ LANGUAGE plpgsql;

CREATE TRIGGER timetravel_audit BEFORE DELETE OR UPDATE
ON timetravel_current FOR EACH ROW EXECUTE PROCEDURE modify_timetravel();

INSERT INTO timetravel(id, f1)
SELECT g.i, 'row-' || g.i::text
FROM generate_series(1,10) AS g(i);

DO $$
  DECLARE
i int;
  BEGIN
FOR i IN 1..2 LOOP
  RAISE NOTICE 'loop = %', i;
  UPDATE timetravel SET f1 = f1 || '-r' || i where id < 2;
END LOOP;
  END
$$;
NOTICE:  loop = 1
NOTICE:  OLD.tr = ["2017-09-03 14:15:08.800811-07",infinity]
NOTICE:  tsr = ["2017-09-03 14:15:08.800811-07","2017-09-03
14:18:48.270274-07")
NOTICE:  NEW.tr = ["2017-09-03 14:18:48.270274-07",infinity]
NOTICE:  loop = 2
NOTICE:  OLD.tr = ["2017-09-03 14:18:48.270274-07",infinity]
NOTICE:  tsr = empty
ERROR:  no partition of relation "timetravel" found for row
DETAIL:  Partition key of the failing row contains (upper(tr)) = (null).
CONTEXT:  SQL statement "INSERT INTO timetravel VALUES (OLD.*)"
PL/pgSQL function modify_timetravel() line 11 at SQL statement
SQL statement "UPDATE timetravel SET f1 = f1 || '-r' || i where id < 2"
PL/pgSQL function inline_code_block line 7 at SQL statement
8<-

Notice that in the first loop iteration tsr is calculated from OLD.tr
correctly. But in the second loop iteration it is not, and therefore no
partition can be found for the insert.

I have not dug too deeply into this yet, but was wondering if this
behavior is sane/expected for some reason I am missing?

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] SCRAM salt length

[Joe Conway]

On 08/17/2017 01:50 PM, Peter Eisentraut wrote:
> On 8/17/17 12:10, Heikki Linnakangas wrote:
>> On 08/17/2017 05:23 PM, Peter Eisentraut wrote:
>>> On 8/17/17 09:21, Heikki Linnakangas wrote:
 The RFC doesn't say anything about salt
 length, but the one example in it uses a 16 byte string as the salt.
 That's more or less equal to the current default of 12 raw bytes, after
 base64-encoding.
>>>
>>> The example is
>>>
>>> S: r=rOprNGfwEbeRWgbNEkqO%hvYDpWUa2RaTCAfuxFIlj)hNlF$k0,
>>>s=W22ZaJ0SNY7soEsUEjb6gQ==,i=4096
>>>
>>> That salt is 24 characters and 16 raw bytes.
>> 
>> Ah, I see, that's from the SCRAM-SHA-256 spec. I was looking at the 
>> example in the original SCRAM-SHA-1 spec:
>> 
>> S: r=fyko+d2lbbFgONRv9qkxdawL3rfcNHYJY1ZVvWVs7j,s=QSXCR+Q6sek8bf92,
>>i=4096
> 
> Hence my original inquiry: "I suspect that this length was chosen based
> on the example in RFC 5802 (SCRAM-SHA-1) section 5.  But the analogous
> example in RFC 7677 (SCRAM-SHA-256) section 3 uses a length of 16.
> Should we use that instead?"

Unless there is some significant downside to using 16 byte salt, that
would be my vote.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] git.postgresql.org (and other services) down

[Joe Conway]

On 08/05/2017 08:36 AM, Joe Conway wrote:
> One of the datacenters that provides two pg.org VM hosts has become
> inaccessible. Services that may be affected are:
> 
> git.postgresql.org
> search.postgresql.org
> Mailinglist archives backend
> PostgreSQL Community Association of Canada
> Download stats analytics database server
> ftpmaster.postgresql.org
> ns1.postgresql.org
> website backend server
> docbot.postgresql.org
> 
> Some of these services have redundant servers, but some, notably
> git.postgresql.org do not.
> 
> I have contacted the provider and will keep the list posted with any
> significant status updates.

Update:

We have not heard back from the provider yet, but access seem to have
been restored and relatively stable at the moment, although there is
still some packet loss.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development




signature.asc
Description: OpenPGP digital signature

[HACKERS] git.postgresql.org (and other services) down

[Joe Conway]

One of the datacenters that provides two pg.org VM hosts has become
inaccessible. Services that may be affected are:

git.postgresql.org
search.postgresql.org
Mailinglist archives backend
PostgreSQL Community Association of Canada
Download stats analytics database server
ftpmaster.postgresql.org
ns1.postgresql.org
website backend server
docbot.postgresql.org

Some of these services have redundant servers, but some, notably
git.postgresql.org do not.

I have contacted the provider and will keep the list posted with any
significant status updates.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Joe Conway]

On 08/02/2017 10:52 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 11:15 PM, Alvaro Herrera
>  wrote:
>> Alvaro Herrera wrote:
>>> I think pg_class is a reasonable place to put more generic relkind lists
>>> alongside a matching error message for each, rather than specialized
>>> "does this relkind have storage" macros.  What about something like a
>>> struct list in pg_class.h,
>>
>> I just noticed that this doesn't help at all with the initial problem
>> statement, which is that some of the relkind checks failed to notice
>> that partitioned tables needed to be added to the set.  Maybe it still
>> helps because you have something to grep for, as Tom proposed elsewhere.
> 
> Having something like relkind_i_t_T, though correct, doesn't make the
> test readable. That's another improvement because of using such
> macros. The macro name tells the purpose of the test, which is what a
> reader would be interested in, rather than the actual values used.

+1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Macros bundling RELKIND_* conditions

[Joe Conway]

On 08/02/2017 10:30 PM, Ashutosh Bapat wrote:
> On Wed, Aug 2, 2017 at 8:47 PM, Robert Haas  wrote:
>> 0001-RELKIND_HAS_VISIBILITY_MAP.patch - one place
>> 0002-RELKIND_HAS_STORAGE.patch - one place
>> 0003-RELKIND_HAS_XIDS-macro.patch - one place
>> 0004-RELKIND_HAS_COMPOSITE_TYPE-macro.patch - one place
>> 0005-RELKIND_CAN_HAVE_TOAST_TABLE-macro.patch - one place
>> 0006-RELKIND_CAN_HAVE_COLUMN_COMMENT-macro.patch - one place
>> 0007-RELKIND_CAN_HAVE_INDEX-macro.patch - two places
>> 0008-RELKIND_CAN_HAVE_COLUMN_SECLABEL-macro.patch - one place
>> 0009-RELKIND_CAN_HAVE_STATS-macro.patch - two places
>>
>> I'm totally cool with doing this where we can use the macro in more
>> than one place, but otherwise I don't think it helps.

I disagree.

> Can we say that any relation that has visibility map will also have
> xids? If yes, what's that common property?

Perhaps there are better ways to achieve the same goal (e.g. nearby
comments), but I think this is the salient point. The macro names allow
whoever is looking at the code to focus on the relevant properties of
the relkind without having to arrive at the conclusion themselves that
relkind "A" has property "B". Makes the code easier to read and
understand IMHO.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] autovacuum can't keep up, bloat just continues to rise

[Joe Conway]

On 07/19/2017 03:29 PM, Tom Lane wrote:
> "Joshua D. Drake"  writes:
>> At PGConf US Philly last week I was talking with Jim and Jan about 
>> performance. One of the items that came up is that PostgreSQL can't run 
>> full throttle for long periods of time. The long and short is that no 
>> matter what, autovacuum can't keep up. This is what I have done:
> 
> Try reducing autovacuum_vacuum_cost_delay more, and/or increasing
> autovacuum_vacuum_cost_limit.

I would try
  autovacuum_vacuum_cost_delay = 0
and for any tables > 1 million rows:
  autovacuum_vacuum_scale_factor: 0
  autovacuum_vacuum_threshold: 10 (perhaps even smaller)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PostgreSQL - Weak DH group

[Joe Conway]

On 07/13/2017 01:07 PM, Simon Riggs wrote:
> On 13 July 2017 at 16:32, Heikki Linnakangas  wrote:
>> (We dropped the ball back in October, continuing the discussion now)
>>
>> On 10/10/2016 06:24 PM, Heikki Linnakangas wrote:
>>>
>>> On 10/06/2016 10:26 PM, Christoph Berg wrote:

 Re: Heikki Linnakangas 2016-10-06
 
>
> I propose the attached patch. It gives up on trying to deal with
> multiple
> key lengths (as noted earlier, OpenSSL just always passed
> keylength=1024, so
> that was useless). Instead of using the callback, it just sets fixed DH
> parameters with SSL_CTX_set_tmp_dh(), like we do for the ECDH curve. The
> DH
> parameters are loaded from a file called "dh_params.pem" (instead of
> "dh1024.pem"), if present, otherwise the built-in 2048 bit parameters
> are
> used.


 Shouldn't this be a GUC pointing to a configurable location like
 ssl_cert_file? This way, people reading the security section of the
 default postgresql.conf would notice that there's something (new) to
 configure. (And I wouldn't want to start creating symlinks from PGDATA
 to /etc/ssl/something again...)
>>>
>>>
>>> Perhaps. The DH parameters are not quite like other configuration files,
>>> though. The actual parameters used don't matter, you just want to
>>> generate different parameters for every cluster. The key length of the
>>> parameters can be considered configuration, though.
>>
>>
>> Actually, adding a GUC also solves another grief I had about this. There is
>> currently no easy way to tell if your parameter file is being used, or if
>> the server is failing to read it and is falling back to the hard-coded
>> parameters. With a GUC, if the GUC is set it's a good indication that the
>> admin expects the file to really exist and to contain valid parameters. So
>> if the GUC is set, we should throw an error if it cannot be used. And if
>> it's not set, we use the built-in defaults.
>>
>> I rebased the patch, did some other clean up of error reporting, and added a
>> GUC along those lines, as well as docs. How does this look?
>>
>> It's late in the release cycle, but it would be nice to sneak this into v10.
>> Using weak 1024 bit DH parameters is arguably a security issue; it was
>> originally reported as such. There's a work-around for older versions:
>> generate custom 2048 bit parameters and place them in a file called
>> "dh1024.pem", but that's completely undocumented.
>>
>> Thoughts? Objections to committing this now, instead of waiting for v11?
> 
> +1 to include important open items such as this

I have not looked at the patch itself, but conceptually +1 to include in
pg10.


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Multi column range partition table

[Joe Conway]

On 07/06/2017 01:24 PM, Dean Rasheed wrote:
> On 6 July 2017 at 21:04, Tom Lane  wrote:
>> Dean Rasheed  writes:
>>> However, this is also an incompatible syntax change, and any attempt
>>> to support both the old and new syntaxes is likely to be messy, so we
>>> really need to get consensus on whether this is the right thing to do,
>>> and whether it *can* be done now for PG10.
>>
>> FWIW, I'd much rather see us get it right the first time than release
>> PG10 with a syntax that we'll regret later.  I do not think that beta2,
>> or even beta3, is too late for such a change.
>>
>> I'm not taking a position on whether this proposal is actually better
>> than what we have.  But if there's a consensus that it is, we should
>> go ahead and do it, not worry that it's too late.
>>
> 
> OK, thanks. That's good to know.

I agree we should get this right the first time and I also agree with
Dean's proposal, so I guess I'm a +2

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] FIPS mode?

[Joe Conway]

On 06/23/2017 10:51 PM, Tom Lane wrote:
> Michael Paquier  writes:
>> On Sat, Jun 24, 2017 at 12:56 PM, Curtis Ruck
>>  wrote:
>>> If I clean this up some, maintain styleguide, what is the likely hood of
>>> getting this included in the redhat packages, since redhat ships a certified
>>> FIPS implementation?
> 
>> So they are applying a custom patch to it already?
> 
> Don't believe so.  It's been a few years since I was at Red Hat, but
> my recollection is that their approach was that it was a system-wide
> configuration choice changing libc's behavior, and there were only very
> minor fixes required to PG's behavior, all of which got propagated
> upstream (see, eg, commit 01824385a).  It sounds like Curtis is trying
> to enable FIPS mode inside Postgres within a system where it isn't enabled
> globally, which according to my recollection has basically nothing to do
> with complying with the actual federal security standard.

Yes, see the PostgreSQL DISA STIG for a discussion with respect to that:

https://www.crunchydata.com/postgres-stig/PGSQL-STIG-9.5+.pdf

HTH,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP: Data at rest encryption

[Joe Conway]

On 06/13/2017 10:20 AM, Stephen Frost wrote:
> * Joe Conway (m...@joeconway.com) wrote:
>> Except shell escaping issues, etc, etc
> 
> That's not an issue- we're talking about reading the stdout of some
> other process, there's no shell escaping that has to be done there.

It could be an issue depending on how the user stores their master key.

> I disagree that proper key management is "simple".  If we really get to
> a point where we think we have a simple answer to it then perhaps that
> can be implemented in addition to the encryption piece in the same
> release cycle- but they certainly don't need to be in the same patch,
> nor do we need to make good key management a requirement for adding
> encryption support.

I never said key management was simple. Indeed it is the most complex
and hazardous part of all this as you said earlier. What is simple is
implementing a master key encrypting actual keys scheme. Keeping the
user's master key management out of this design is unchanged by what I
proposed, and what I proposed is a superior yet simple method. Yes, it
can be done separately but what is the point? We should at least discuss
it as part of the design.

> No, but it seriously changes the level of complexity.  I feel like we're
> trying to go from zero to light speed here because there's an idea that
> it's "simple" to add X, Y or Z additional requirement beyond the basic
> feature, but we don't have anything yet.

I think that is hyperbole. It does not significantly add to the
complexity of what is being discussed.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP: Data at rest encryption

[Joe Conway]

On 06/13/2017 10:05 AM, Stephen Frost wrote:
> Bruce, Joe,
> 
> * Bruce Momjian (br...@momjian.us) wrote:
>> On Tue, Jun 13, 2017 at 09:55:10AM -0700, Joe Conway wrote:
>> > > That way, if the user wants to store the key in an unencrypted text
>> > > file, they can set the encryption_key_command = 'cat /not/very/secure'
>> > > and call it a day.  If they want to prompt the user on the console or
>> > > request the key from an HSM or get it in any other way, they just have
>> > > to write the appropriate shell script.  We just provide mechanism, not
>> > > policy, and the user can adopt any policy they like, from an extremely
>> > > insecure policy to one suitable for Fort Knox.
>> > 
>> > Agreed, but as Bruce alluded to, we want this to be a master key, which
>> > is in turn used to encrypt the actual key, or keys, that are used to
>> > encrypt the data. The actual data encryption keys could be very long
>> > randomly generated binary, and there could be more than one of them
>> > (e.g. one per tablespace) in a file which is encrypted with the master
>> > key. This is more secure and allows, for example the master key to be
>> > changed without having to decrypt/re-encrypt the entire database.
>> 
>> Yes, thank you.  Also, you can make multiple RSA-encrypted copies of the
>> symetric key, one for each role you want to view the data.  And good
>> point on the ability to change the RSA key/password without having to
>> reencrypt the data.
> 
> There's nothing in this proposal that prevents the user from using a
> very long randomly generated binary key.  We aren't talking about
> prompting the user for a password unless that's what they decide the
> shell script should do, unless the user decides to do that and if they
> do then that's their choice.

Except shell escaping issues, etc, etc

> Let us, please, stop stressing over the right way to do key management
> as part of this discussion about providing encryption.  The two are
> different things and we do not need to solve both at once.

Not stressing, but this is an important part of the design and should be
done correctly. It is also very simple, so should not be hard to add.

> Further, yes, we will definitely want to get to a point where we can
> encrypt subsets of the system in different ways, but that doesn't have
> to be done in the first implementation either.

No, it doesn't, but that doesn't change the utility of doing it this way
from the start.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] WIP: Data at rest encryption

[Joe Conway]

On 06/13/2017 09:28 AM, Robert Haas wrote:
> On Tue, Jun 13, 2017 at 12:23 PM, Stephen Frost  wrote:
>> Key management is an entirely independent discussion from this and the
>> proposal from Ants, as I understand it, is that the key would *not* be
>> in the database but could be anywhere that a shell command could get it
>> from, including possibly a HSM (hardware device).
> 
> Yes.  I think the right way to implement this is something like:
> 
> 1. Have a GUC that runs a shell command to get the key.
> 
> 2. If the command successfully gets the key, it prints it to stdout
> and returns 0.
> 
> 3. If it doesn't get successfully get the key, it returns 1.  The
> database can retry or give up, whatever we decide to do.
> 
> That way, if the user wants to store the key in an unencrypted text
> file, they can set the encryption_key_command = 'cat /not/very/secure'
> and call it a day.  If they want to prompt the user on the console or
> request the key from an HSM or get it in any other way, they just have
> to write the appropriate shell script.  We just provide mechanism, not
> policy, and the user can adopt any policy they like, from an extremely
> insecure policy to one suitable for Fort Knox.

Agreed, but as Bruce alluded to, we want this to be a master key, which
is in turn used to encrypt the actual key, or keys, that are used to
encrypt the data. The actual data encryption keys could be very long
randomly generated binary, and there could be more than one of them
(e.g. one per tablespace) in a file which is encrypted with the master
key. This is more secure and allows, for example the master key to be
changed without having to decrypt/re-encrypt the entire database.

Joe


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/12/2017 11:33 AM, Dean Rasheed wrote:
> On 12 June 2017 at 17:51, Joe Conway <m...@joeconway.com> wrote:
>> After looking I remain convinced - +1 in general.
> 
> Yes, I think this will probably help, but I worry that it will turn
> into quite a large and invasive patch, and there are a number of
> design choices to be made over the naming and precise set of macros.
> Is this really PG10 material?


I was wondering the same after responding. Possibly not.


> My initial thought, looking at the patch, is that it might be better
> to have all the macros in one file to make them easier to maintain.


Yeah, that was my thought as well.


>> sync in the future. Maybe something like this:
>> 8<-
>>"\"%s\" is not a kind of relation that can have column comments"
>> 8<-
>> Thoughts?
> 
> -1. I think the existing error messages provide useful information
> that you'd be removing. If you're worried about the error messages
> getting out of sync, then wouldn't it be better to centralise them
> along with the corresponding macros?


I guess that could work too.


> Barring objections, I'll push my original patch and work up patches
> for the other couple of issues I found.


No objections here -- we definitely need to fix those.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/12/2017 07:40 AM, Joe Conway wrote:
> On 06/12/2017 01:49 AM, Amit Langote wrote:
>>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed <dean.a.rash...@gmail.com> 
>>> wrote:
>>>> It looks like relation_is_updatable() didn't get the message about
>>>> partitioned tables. Thus, for example, information_schema.views and
>>>> information_schema.columns report that simple views built on top of
>>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>>> patch to fix this.
>> 
>> Thanks for the patch, Dean.
>> 
>>>> I think this kind of omission is an easy mistake to make when adding a
>>>> new relkind, so it might be worth having more pairs of eyes looking
>>>> out for more of the same. I did a quick scan of the rewriter code
>>>> (prompted by the recent similar omission for RLS on partitioned
>>>> tables) and I didn't find any more problems there, but I haven't
>>>> looked elsewhere yet.
>> 
>> As he mentioned in his reply, Ashutosh's proposal to abstract away the
>> relkind checks is interesting in this regard.
>> 
>> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>>> Changes look good to me. In order to avoid such instances in future, I
>>> have proposed to bundle the conditions as macros in [1].
>> 
>> It seems that Ashutosh forgot to include the link:
>> 
>> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com
> 
> I have not looked at Ashutosh's patch yet, but it sounds like a good
> idea to me.

After looking I remain convinced - +1 in general.

One thing I would change -- in many cases there are ERROR messages
enumerating the relkinds. E.g.:
8<-
if (!RELKIND_CAN_HAVE_COLUMN_COMMENT(relation->rd_rel->relkind))
ereport(ERROR,
(errcode(ERRCODE_WRONG_OBJECT_TYPE),
errmsg("\"%s\" is not a table, view, materialized view,
composite type, or foreign table",
8<-

I think those messages should also be rewritten to make them less
relkind specific and more clear, otherwise we still risk getting out of
sync in the future. Maybe something like this:
8<-
   "\"%s\" is not a kind of relation that can have column comments"
8<-
Thoughts?

In any case, unless another committer else wants it, and assuming no
complaints with the concept, I'd be happy to take this.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/12/2017 01:49 AM, Amit Langote wrote:
>> On Sun, Jun 11, 2017 at 5:02 PM, Dean Rasheed  
>> wrote:
>>> It looks like relation_is_updatable() didn't get the message about
>>> partitioned tables. Thus, for example, information_schema.views and
>>> information_schema.columns report that simple views built on top of
>>> partitioned tables are non-updatable, which is wrong. Attached is a
>>> patch to fix this.
> 
> Thanks for the patch, Dean.
> 
>>> I think this kind of omission is an easy mistake to make when adding a
>>> new relkind, so it might be worth having more pairs of eyes looking
>>> out for more of the same. I did a quick scan of the rewriter code
>>> (prompted by the recent similar omission for RLS on partitioned
>>> tables) and I didn't find any more problems there, but I haven't
>>> looked elsewhere yet.
> 
> As he mentioned in his reply, Ashutosh's proposal to abstract away the
> relkind checks is interesting in this regard.
> 
> On 2017/06/12 17:29, Ashutosh Bapat wrote:
>> Changes look good to me. In order to avoid such instances in future, I
>> have proposed to bundle the conditions as macros in [1].
> 
> It seems that Ashutosh forgot to include the link:
> 
> https://www.postgresql.org/message-id/CAFjFpRcfzs+yst6YBCseD_orEcDNuAr9GUTraZ5GC=avcyh...@mail.gmail.com

I have not looked at Ashutosh's patch yet, but it sounds like a good
idea to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/11/2017 08:55 AM, Joe Conway wrote:
> On 06/11/2017 04:32 AM, Dean Rasheed wrote:
>> It looks like relation_is_updatable() didn't get the message about
>> partitioned tables. Thus, for example, information_schema.views and
>> information_schema.columns report that simple views built on top of
>> partitioned tables are non-updatable, which is wrong. Attached is a
>> patch to fix this.
> 
>> I think this kind of omission is an easy mistake to make when adding a
>> new relkind, so it might be worth having more pairs of eyes looking
>> out for more of the same. I did a quick scan of the rewriter code
>> (prompted by the recent similar omission for RLS on partitioned
>> tables) and I didn't find any more problems there, but I haven't
>> looked elsewhere yet.
> 
> Yeah, I noticed the same while working on the RLS related patch. I did
> not see anything else in rewriteHandler.c but it is probably worth
> looking wider for other omissions.

So in particular:

src/include/utils/rel.h:318:
8<
/*
 * RelationIsUsedAsCatalogTable
 *Returns whether the relation should be treated as a catalog table
 *from the pov of logical decoding.  Note multiple eval of argument!
 */
#define RelationIsUsedAsCatalogTable(relation)  \
((relation)->rd_options && \
 ((relation)->rd_rel->relkind == RELKIND_RELATION || \
  (relation)->rd_rel->relkind == RELKIND_MATVIEW) ? \
 ((StdRdOptions *) (relation)->rd_options)->user_catalog_table : false)
8<

Does RELKIND_PARTITIONED_TABLE apply there?

Will continue to poke around...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] PG10 Partitioned tables and relation_is_updatable()

[Joe Conway]

On 06/11/2017 04:32 AM, Dean Rasheed wrote:
> It looks like relation_is_updatable() didn't get the message about
> partitioned tables. Thus, for example, information_schema.views and
> information_schema.columns report that simple views built on top of
> partitioned tables are non-updatable, which is wrong. Attached is a
> patch to fix this.

> I think this kind of omission is an easy mistake to make when adding a
> new relkind, so it might be worth having more pairs of eyes looking
> out for more of the same. I did a quick scan of the rewriter code
> (prompted by the recent similar omission for RLS on partitioned
> tables) and I didn't find any more problems there, but I haven't
> looked elsewhere yet.

Yeah, I noticed the same while working on the RLS related patch. I did
not see anything else in rewriteHandler.c but it is probably worth
looking wider for other omissions.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/09/2017 02:52 PM, Joe Conway wrote:
> On 06/09/2017 06:16 AM, Joe Conway wrote:
>> On 06/08/2017 11:09 PM, Noah Misch wrote:
>>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>>>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>>>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>>>> > in order to demonstrate policy sorting and order of RLS/partition
>>>> > constraint checking. It should be much more straight-forward now, but
>>>> > let me know if there are any further recommended changes.
>>>> 
>>>> Thanks, will take a look towards the end of the day.
>>> 
>>> This PostgreSQL 10 open item is past due for your status update.  Kindly 
>>> send
>>> a status update within 24 hours, and include a date for your subsequent 
>>> status
>>> update.  Refer to the policy on open item ownership:
>>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
>> 
>> I started reviewing the latest patch last night and will try to finish
>> up this afternoon (west coast USA time).
> 
> I left the actual (2 line) code change untouched, but I tweaked the
> regression test changes a bit. If there are no complaints I will push
> tomorrow (Saturday).

I waited until Sunday, but pushed none-the-less.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/09/2017 06:16 AM, Joe Conway wrote:
> On 06/08/2017 11:09 PM, Noah Misch wrote:
>> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>>> > in order to demonstrate policy sorting and order of RLS/partition
>>> > constraint checking. It should be much more straight-forward now, but
>>> > let me know if there are any further recommended changes.
>>> 
>>> Thanks, will take a look towards the end of the day.
>> 
>> This PostgreSQL 10 open item is past due for your status update.  Kindly send
>> a status update within 24 hours, and include a date for your subsequent 
>> status
>> update.  Refer to the policy on open item ownership:
>> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com
> 
> I started reviewing the latest patch last night and will try to finish
> up this afternoon (west coast USA time).

I left the actual (2 line) code change untouched, but I tweaked the
regression test changes a bit. If there are no complaints I will push
tomorrow (Saturday).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/src/backend/rewrite/rewriteHandler.c b/src/backend/rewrite/rewriteHandler.c
index 35ff8bb..6cd73c1 100644
*** a/src/backend/rewrite/rewriteHandler.c
--- b/src/backend/rewrite/rewriteHandler.c
*** fireRIRrules(Query *parsetree, List *act
*** 1835,1841 
  
  		/* Only normal relations can have RLS policies */
  		if (rte->rtekind != RTE_RELATION ||
! 			rte->relkind != RELKIND_RELATION)
  			continue;
  
  		rel = heap_open(rte->relid, NoLock);
--- 1835,1842 
  
  		/* Only normal relations can have RLS policies */
  		if (rte->rtekind != RTE_RELATION ||
! 			(rte->relkind != RELKIND_RELATION &&
! 			rte->relkind != RELKIND_PARTITIONED_TABLE))
  			continue;
  
  		rel = heap_open(rte->relid, NoLock);
diff --git a/src/test/regress/expected/rowsecurity.out b/src/test/regress/expected/rowsecurity.out
index 7bf2936..d382a9f 100644
*** a/src/test/regress/expected/rowsecurity.out
--- b/src/test/regress/expected/rowsecurity.out
*** EXPLAIN (COSTS OFF) SELECT * FROM t1 WHE
*** 899,904 
--- 899,1332 
   Filter: f_leak(b)
  (7 rows)
  
+ --
+ -- Partitioned Tables
+ --
+ SET SESSION AUTHORIZATION regress_rls_alice;
+ CREATE TABLE part_document (
+ did int,
+ cid int,
+ dlevel  int not null,
+ dauthor name,
+ dtitle  text
+ ) PARTITION BY RANGE (cid);
+ GRANT ALL ON part_document TO public;
+ -- Create partitions for document categories
+ CREATE TABLE part_document_fiction PARTITION OF part_document FOR VALUES FROM (11) to (12);
+ CREATE TABLE part_document_satire PARTITION OF part_document FOR VALUES FROM (55) to (56);
+ CREATE TABLE part_document_nonfiction PARTITION OF part_document FOR VALUES FROM (99) to (100);
+ GRANT ALL ON part_document_fiction TO public;
+ GRANT ALL ON part_document_satire TO public;
+ GRANT ALL ON part_document_nonfiction TO public;
+ INSERT INTO part_document VALUES
+ ( 1, 11, 1, 'regress_rls_bob', 'my first novel'),
+ ( 2, 11, 2, 'regress_rls_bob', 'my second novel'),
+ ( 3, 99, 2, 'regress_rls_bob', 'my science textbook'),
+ ( 4, 55, 1, 'regress_rls_bob', 'my first satire'),
+ ( 5, 99, 2, 'regress_rls_bob', 'my history book'),
+ ( 6, 11, 1, 'regress_rls_carol', 'great science fiction'),
+ ( 7, 99, 2, 'regress_rls_carol', 'great technology book'),
+ ( 8, 55, 2, 'regress_rls_carol', 'great satire'),
+ ( 9, 11, 1, 'regress_rls_dave', 'awesome science fiction'),
+ (10, 99, 2, 'regress_rls_dave', 'awesome technology book');
+ ALTER TABLE part_document ENABLE ROW LEVEL SECURITY;
+ -- Create policy on parent
+ -- user's security level must be higher than or equal to document's
+ CREATE POLICY pp1 ON part_document AS PERMISSIVE
+ USING (dlevel <= (SELECT seclv FROM uaccount WHERE pguser = current_user));
+ -- Dave is only allowed to see cid < 55
+ CREATE POLICY pp1r ON part_document AS RESTRICTIVE TO regress_rls_dave
+ USING (cid < 55);
+ \d+ part_document
+   Table "regress_rls_schema.part_document"
+  Column  |  Type   | Collation | Nullable | Default | Storage  | Stats target | Description 
+ -+-+---+--+-+--+--+-
+  did | integer |   |  | | plain|  | 
+  cid | integer |   |  | | plain|  | 
+  dlevel  | integer |   | not null | | plain|  | 
+  dauthor | name|   | 

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/08/2017 11:09 PM, Noah Misch wrote:
> On Wed, Jun 07, 2017 at 08:45:20AM -0700, Joe Conway wrote:
>> On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
>> > I ended up narrowing it down to 4 tables (one parent and 3 partitions)
>> > in order to demonstrate policy sorting and order of RLS/partition
>> > constraint checking. It should be much more straight-forward now, but
>> > let me know if there are any further recommended changes.
>> 
>> Thanks, will take a look towards the end of the day.
> 
> This PostgreSQL 10 open item is past due for your status update.  Kindly send
> a status update within 24 hours, and include a date for your subsequent status
> update.  Refer to the policy on open item ownership:
> https://www.postgresql.org/message-id/20170404140717.GA2675809%40tornado.leadboat.com

I started reviewing the latest patch last night and will try to finish
up this afternoon (west coast USA time).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/07/2017 06:49 AM, Mike Palmiotto wrote:
> I ended up narrowing it down to 4 tables (one parent and 3 partitions)
> in order to demonstrate policy sorting and order of RLS/partition
> constraint checking. It should be much more straight-forward now, but
> let me know if there are any further recommended changes.

Thanks, will take a look towards the end of the day.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/06/2017 02:44 PM, Mike Palmiotto wrote:
> On Tue, Jun 6, 2017 at 4:07 PM, Joe Conway <m...@joeconway.com> wrote:
>> On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
>>> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmh...@gmail.com> wrote:
>>>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <m...@joeconway.com> wrote:
>>>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>>>> committed in the next day or two.
>>>>
>>>> That would be great.  Thanks.
>>>
>>> I have the updated patch with rowsecurity regression tests and rebased
>>> on master. I've run these and verified locally by feeding
>>> rowsecurity.sql to psql, but have yet to get the full regression suite
>>> passing -- it's failing on the constraints regtest and then gets stuck
>>> in recovery. Undoubtedly something to do with my
>>> configuration/environment over here. I'm working through those issues
>>> right now. In the meantime, if you want to see the regression tests as
>>> they stand, please see the attached patch.
>>
>> The constraints test passes here, so presumably something you borked
>> locally. I only see a rowsecurity failure, which is not surprising since
>> your patch does not include the changes to expected output ;-)
>> Please resend with src/test/regress/expected/rowsecurity.out included.
> 
> It was indeed an issue on my end. Attached are the rowsecurity
> regression tests and the expected out. Unsurprisingly, all tests pass,
> because I said so. :)
> 
> Let me know if you want me to make any revisions.


Thanks Mike. I'll take a close look to verify output correctnes, but I
am concerned that the new tests are unnecessarily complex. Any other
opinions on that?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/06/2017 11:57 AM, Mike Palmiotto wrote:
> On Mon, Jun 5, 2017 at 10:36 AM, Robert Haas <robertmh...@gmail.com> wrote:
>> On Mon, Jun 5, 2017 at 10:20 AM, Joe Conway <m...@joeconway.com> wrote:
>>> Unless Robert objects, I'll work with Mike to get a fix posted and
>>> committed in the next day or two.
>>
>> That would be great.  Thanks.
> 
> I have the updated patch with rowsecurity regression tests and rebased
> on master. I've run these and verified locally by feeding
> rowsecurity.sql to psql, but have yet to get the full regression suite
> passing -- it's failing on the constraints regtest and then gets stuck
> in recovery. Undoubtedly something to do with my
> configuration/environment over here. I'm working through those issues
> right now. In the meantime, if you want to see the regression tests as
> they stand, please see the attached patch.

The constraints test passes here, so presumably something you borked
locally. I only see a rowsecurity failure, which is not surprising since
your patch does not include the changes to expected output ;-)
Please resend with src/test/regress/expected/rowsecurity.out included.

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [BUGS] BUG #14682: row level security not work with partitioned table

[Joe Conway]

On 06/04/2017 03:33 PM, Noah Misch wrote:
> On Fri, Jun 02, 2017 at 09:28:16AM +0900, Michael Paquier wrote:
>> On Thu, Jun 1, 2017 at 11:13 AM, Mike Palmiotto
>>  wrote:
>> > This is indeed a bug. fireRIRrules is currently skipping the RLS
>> > policy check when relkind == PARTITIONED_TABLES, so RLS policies are
>> > not applied. The attached patch fixes the behavior.
>> 
>> I would expect RLS to trigger as well in this context. Note that there
>> should be regression tests to avoid this failure again in the future.
>> I have added an open item.
> 
> [Action required within three days.  This is a generic notification.]
> 
> The above-described topic is currently a PostgreSQL 10 open item.  Robert,
> since you committed the patch believed to have created it, you own this open
> item.

Unless Robert objects, I'll work with Mike to get a fix posted and
committed in the next day or two.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Hash Functions

[Joe Conway]

On 06/02/2017 05:47 AM, Robert Haas wrote:
> On Fri, Jun 2, 2017 at 1:24 AM, Jeff Davis  wrote:
>> 2. I basically see two approaches to solve the problem:
>>   (a) Tom suggested at PGCon that we could have a GUC that
>> automatically causes inserts to the partition to be re-routed through
>> the parent. We could discuss whether to always route through the
>> parent, or do a recheck on the partition constrains and only reroute
>> tuples that will fail it. If the user gets into trouble, the worst
>> that would happen is a helpful error message telling them to set the
>> GUC. I like this idea.
> 
> Yeah, that's not crazy.  I find it a bit surprising in terms of the
> semantics, though.  SET
> when_i_try_to_insert_into_a_specific_partition_i_dont_really_mean_it =
> true?

Maybe
  SET partition_tuple_retry = true;
-or-
  SET partition_tuple_reroute = true;
?

I like the idea of only rerouting when failing constraints although I
can envision where there might be use cases where you essentially want
to re-partition and therefore reroute everything, leading to:

  SET partition_tuple_reroute = (none | error | all);

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Hash Functions

[Joe Conway]

On 06/01/2017 11:25 AM, Andres Freund wrote:
> On 2017-06-01 13:59:42 -0400, Robert Haas wrote:
>> My personal guess is that most people will prefer the fast
>> hash functions over the ones that solve their potential future
>> migration problems, but, hey, options are good.
> 
> I'm pretty sure that will be the case.  I'm not sure that adding
> infrastructure to allow for something that nobody will use in practice
> is a good idea.  If there ends up being demand for it, we can still go there.
> 
> I think that the number of people that migrate between architectures is
> low enough that this isn't going to be a very common issue.  Having some
> feasible way around this is important, but I don't think we should
> optimize heavily for it by developing new infrastructure / complicating
> experience for the 'normal' uses.

+1

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Use of non-restart-safe storage by temp_tablespaces

[Joe Conway]

On 05/30/2017 10:54 AM, Tom Lane wrote:
> Bruce Momjian <br...@momjian.us> writes:
>> On Mon, May 29, 2017 at 03:55:08PM -0400, Tom Lane wrote:
>>> I'm too lazy to search the archives right now, but there was some case
>>> years ago where somebody destroyed their database via an ill-thought-out
>>> combination of automatic-initdb-if-$PGDATA-isn't-there and slow mounting.
> 
>> Here is the Joe Conway report from 2004:
>>  https://postgr.es/m/41bfab7c.5040...@joeconway.com
> 
> Yeah, that's the thread I was remembering.  Thanks for looking it up.

Wow, reading that was a blast from the past!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Hash Functions

[Joe Conway]

On 05/12/2017 10:17 AM, Robert Haas wrote:
> On Fri, May 12, 2017 at 1:12 PM, Andres Freund wrote:
>> Given that a lot of data types have a architecture dependent
>> representation, it seems somewhat unrealistic and expensive to have
>> a hard rule to keep them architecture agnostic.   And if that's not
>> guaranteed, then I'm doubtful it makes sense as a soft rule
>> either.
> 
> That's a good point, but the flip side is that, if we don't have
> such a rule, a pg_dump of a hash-partitioned table on one
> architecture might fail to restore on another architecture.  Today, I
> believe that, while the actual database cluster is
> architecture-dependent, a pg_dump is architecture-independent.  Is it
> OK to lose that property?


Not from where I sit.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Should pg_current_wal_location() become pg_current_wal_lsn()

[Joe Conway]

On 05/10/2017 12:22 PM, Tom Lane wrote:
> Robert Haas  writes:
>> On Wed, May 10, 2017 at 1:13 PM, Tom Lane  wrote:
>>> In terms of the alternatives I listed previously, it seems like
>>> nobody liked alternatives #3, #4, or #5, leaving us with #1 (do
>>> nothing) or #2 (apply this patch).  By my count, Peter is the
>>> only one in favor of doing nothing, and is outvoted.  I'll push
>>> the patch later today if I don't hear additional comments.
> 
>> For the record, I also voted for doing nothing.
> 
> Hm, well, anybody else want to vote?

+1 for #2

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 07:04 PM, Tom Lane wrote:
> Craig Ringer  writes:
>> We're carefully maintaining this bizarre cognitive dissonance where we
>> justify the need for using this as a planner hint at the same time as
>> denying that we have a hint. That makes it hard to make progress here.
>> I think there's fear that we're setting some kind of precedent by
>> admitting what we already have.
> 
> I think you're overstating the case.  It's clear that there's a
> significant subset of CTE functionality where there has to be an
> optimization fence.  The initial implementation basically took the
> easy way out by deeming *all* CTEs to be optimization fences.  Maybe
> we shouldn't have documented that behavior, but we did.  Now we're
> arguing about how much of a compatibility break it'd be to change that
> planner behavior.  I don't see any particular cognitive dissonance here,
> just disagreements about the extent to which backwards compatibility is
> more important than better query optimization.

Exactly.

One thought, is that we treat a CTE in a similar way to foreign tables,
with the same set of push downs.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 05:03 PM, Craig Ringer wrote:
> On 5 May 2017 02:52, "Tom Lane" wrote:
> I haven't been keeping close tabs either, but surely we still have
> to have
> the optimization fence in (at least) all these cases:
> 
> * CTE contains INSERT/UPDATE/DELETE
> * CTE contains SELECT FOR UPDATE/SHARE (else the set of rows that get
>   locked might change)
> * CTE contains volatile functions
> 
> I'm willing to write off cases where, eg, a function should have been
> marked volatile and was not.  That's user error and there are plenty
> of hazards of that kind already.  But if the optimizer has reason
> to know that discarding the fence might change any query side-effects,
> it mustn't.
> 
> I think everyone is in total agreement there. 

That's great, but if so, why do we need any change in syntax at all?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 10:56 AM, Andrew Dunstan wrote:
> 
> 
> On 05/04/2017 01:52 PM, Joe Conway wrote:
>> On 05/04/2017 10:33 AM, Alvaro Herrera wrote:
>>> I'm not sure what your point is.  We know that for some cases the
>>> optimization barrier semantics are useful, which is why the proposal is
>>> to add a keyword to install one explicitely:
>>>
>>>  with materialized r as
>>>  (
>>> select json_populate_record(null::mytype, myjson) as x
>>> from mytable
>>>  )
>>>  select (x).*
>>>  from r;
>>>
>>> this would preserve the current semantics.
>> I haven't been able to follow this incredibly long thread, so please
>> excuse me if way off base, but are we talking about that a CTE would be
>> silently be rewritten as an inline expression potentially unless it is
>> decorated with some new syntax?
>>
>> I would find that very disconcerting myself. For example, would this CTE
>> potentially get rewritten with multiple evaluation as follows?
>>
>> DROP SEQUENCE IF EXISTS foo_seq;
>> CREATE SEQUENCE foo_seq;
>>
>> WITH a(f1) AS (SELECT nextval('foo_seq'))
>> SELECT a.f1, a.f1 FROM a;
>>  f1 | ?column?
>> +--
>>   1 |1
>> (1 row)
>>
>> ALTER SEQUENCE foo_seq RESTART;
>> SELECT nextval('foo_seq'), nextval('foo_seq');
>>  nextval | ?column?
>> -+--
>>1 |2
>> (1 row)
>>
> 
> I think that would be a change in semantics, which we should definitely
> not be getting. Avoiding a change in semantics might be an interesting
> exercise, but we have lots of clever coders ...

Well I think my point is that I always have understood CTEs to be
executed precisely once producing a temporary result set that is then
referenced elsewhere. I don't think that property of CTEs should change.
Somewhere else in the thread someone mentioned predicate push down --
that makes sense and maybe some clever coder can come up with a patch
that does that, but I would not be in favor of CTEs being inlined and
therefore evaluated multiple times.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CTE inlining

[Joe Conway]

On 05/04/2017 10:33 AM, Alvaro Herrera wrote:
> I'm not sure what your point is.  We know that for some cases the
> optimization barrier semantics are useful, which is why the proposal is
> to add a keyword to install one explicitely:
> 
>  with materialized r as
>  (
> select json_populate_record(null::mytype, myjson) as x
> from mytable
>  )
>  select (x).*
>  from r;
> 
> this would preserve the current semantics.

I haven't been able to follow this incredibly long thread, so please
excuse me if way off base, but are we talking about that a CTE would be
silently be rewritten as an inline expression potentially unless it is
decorated with some new syntax?

I would find that very disconcerting myself. For example, would this CTE
potentially get rewritten with multiple evaluation as follows?

DROP SEQUENCE IF EXISTS foo_seq;
CREATE SEQUENCE foo_seq;

WITH a(f1) AS (SELECT nextval('foo_seq'))
SELECT a.f1, a.f1 FROM a;
 f1 | ?column?
+--
  1 |1
(1 row)

ALTER SEQUENCE foo_seq RESTART;
SELECT nextval('foo_seq'), nextval('foo_seq');
 nextval | ?column?
-+--
   1 |2
(1 row)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Row Level Security UPDATE Confusion

[Joe Conway]

On 04/13/2017 01:31 PM, Stephen Frost wrote:
> * Robert Haas (robertmh...@gmail.com) wrote:
>> On Thu, Apr 6, 2017 at 4:05 PM, Rod Taylor  wrote:
>> > I'm a little confused on why a SELECT policy fires against the NEW record
>> > for an UPDATE when using multiple FOR policies. The ALL policy doesn't seem
>> > to have that restriction.
>> 
>> My guess is that you have found a bug.
> 
> Indeed.  Joe's been looking into it and I'm hoping to find some time to
> dig into it shortly.


>> CREATE POLICY split_select ON t FOR SELECT TO split
>> USING (value > 0);
>> CREATE POLICY split_update ON t FOR UPDATE TO split
>> USING (true) WITH CHECK (true);

Yes -- from what I can see in gdb:

1) add_with_check_options() adds both (value > 0) and (true) to
   withCheckOptions -- this seems correct as the USING expression
   is used for WITH CHECK when the latter is not specified

2) ExecWithCheckOptions() checks (value > 0) which fails, and it
   immediately throws an ERROR, i.e. it never checks the (true)
   expression and therefore never ORs the results -- this seems
   incorrect, it uses restrictive not permissive

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 04:14 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>>> I turned on the buildfarm "keep" setting and looked at the diffs. The
>>> issue is that in there are a few places that do "SELECT ... FROM
>>> pg_seclabels ... ORDER BY ..." and when manually testing I get default
>>> database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
>>> a different sorting.
> 
>> Looks like adding COLLATE "C" in the ORDER BY clauses does the trick. Is
>> that the preferred method for this kind of issue?
> 
> Yeah, that's pretty much the standard fix nowadays.


Good to hear, thanks -- rhino is back to green.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 03:01 PM, Joe Conway wrote:
> On 04/09/2017 02:49 PM, Andres Freund wrote:
>> On 2017-04-09 14:28:48 -0700, Joe Conway wrote:
>>> Interesting -- rhino is now failing. I tested a minute ago manually on
>>> the same buildfarm animal and it passed. I'm on it.
>> 
>> The module for segpsql really needs to be improved so it logs
>> regression.diffs - iirc several other modules do.
> 
> Sure, but for another day I think.
> 
> I turned on the buildfarm "keep" setting and looked at the diffs. The
> issue is that in there are a few places that do "SELECT ... FROM
> pg_seclabels ... ORDER BY ..." and when manually testing I get default
> database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
> a different sorting.

Looks like adding COLLATE "C" in the ORDER BY clauses does the trick. Is
that the preferred method for this kind of issue?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 02:49 PM, Andres Freund wrote:
> On 2017-04-09 14:28:48 -0700, Joe Conway wrote:
>> Interesting -- rhino is now failing. I tested a minute ago manually on
>> the same buildfarm animal and it passed. I'm on it.
> 
> The module for segpsql really needs to be improved so it logs
> regression.diffs - iirc several other modules do.

Sure, but for another day I think.

I turned on the buildfarm "keep" setting and looked at the diffs. The
issue is that in there are a few places that do "SELECT ... FROM
pg_seclabels ... ORDER BY ..." and when manually testing I get default
database encoding "UTF8" but with the buildfarm I get "SQL_ASCII", hence
a different sorting.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/09/2017 02:04 PM, Joe Conway wrote:
> On 04/08/2017 07:29 AM, Stephen Frost wrote:
>> * Joe Conway (m...@joeconway.com) wrote:
>>> On 04/07/2017 05:36 PM, Robert Haas wrote:
>>> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <m...@joeconway.com> wrote:
>>> >> 1) commit the 0002 patch now before the feature freeze and follow up
>>> >>with the regression test patch when ready in a couple of days
>>> >> 2) hold off on both patches until ready
>>> >> 3) push both patches to the next commitfest/pg11
>>> >>
>>> >> Some argue this is an open issue against the new partitioning feature in
>>> >> pg10 and therefore should be addressed now, and others do not. I can see
>>> >> both sides of that argument.
>>> >>
>>> >> In any case, thoughts on what to do?
>>> > 
>>> > Speaking only for myself, I'm OK with any of those options, provided
>>> > that that "a couple" means what my dictionary says it means.
>>> 
>>> Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
>>> the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
>>> are ready by Sunday I will push both together (#2) or else I will move
>>> them both together to the next CF (#3).
>> 
>> Sounds good to me.
> 
> Having heard no objections, committed and pushed as per option #2.

Interesting -- rhino is now failing. I tested a minute ago manually on
the same buildfarm animal and it passed. I'm on it.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/08/2017 07:29 AM, Stephen Frost wrote:
> * Joe Conway (m...@joeconway.com) wrote:
>> On 04/07/2017 05:36 PM, Robert Haas wrote:
>> > On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <m...@joeconway.com> wrote:
>> >> 1) commit the 0002 patch now before the feature freeze and follow up
>> >>with the regression test patch when ready in a couple of days
>> >> 2) hold off on both patches until ready
>> >> 3) push both patches to the next commitfest/pg11
>> >>
>> >> Some argue this is an open issue against the new partitioning feature in
>> >> pg10 and therefore should be addressed now, and others do not. I can see
>> >> both sides of that argument.
>> >>
>> >> In any case, thoughts on what to do?
>> > 
>> > Speaking only for myself, I'm OK with any of those options, provided
>> > that that "a couple" means what my dictionary says it means.
>> 
>> Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
>> the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
>> are ready by Sunday I will push both together (#2) or else I will move
>> them both together to the next CF (#3).
> 
> Sounds good to me.

Having heard no objections, committed and pushed as per option #2.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/07/2017 05:36 PM, Robert Haas wrote:
> On Fri, Apr 7, 2017 at 5:22 PM, Joe Conway <m...@joeconway.com> wrote:
>> 1) commit the 0002 patch now before the feature freeze and follow up
>>with the regression test patch when ready in a couple of days
>> 2) hold off on both patches until ready
>> 3) push both patches to the next commitfest/pg11
>>
>> Some argue this is an open issue against the new partitioning feature in
>> pg10 and therefore should be addressed now, and others do not. I can see
>> both sides of that argument.
>>
>> In any case, thoughts on what to do?
> 
> Speaking only for myself, I'm OK with any of those options, provided
> that that "a couple" means what my dictionary says it means.

Thanks. I'd prefer not to do #1 actually, and I think we can adhere to
the dictionary meaning of "a couple" (i.e. by EOD Sunday). Assuming we
are ready by Sunday I will push both together (#2) or else I will move
them both together to the next CF (#3).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/07/2017 11:37 AM, Mike Palmiotto wrote:
>>> I found some missing bits in the 0002 patch -- new version attached.
>>> Will wait on new regression tests before committing, but I expect we'll
>>> have those by end of today and be able to commit the rest tomorrow.
>>
>> Attached are the regression test updates for partitioned tables.
> 
> Actually attached this time.

Based on my review and testing of the 0002 patch I believe it is
correct. However Mike and I just went through the regression test patch
line by line and there are issues he needs to address -- there is no way
that is happening by tonight as the output is very verbose and we need
to be sure we are both testing the correct things and getting the
correct behaviors.

Based on that I can:

1) commit the 0002 patch now before the feature freeze and follow up
   with the regression test patch when ready in a couple of days
2) hold off on both patches until ready
3) push both patches to the next commitfest/pg11

Some argue this is an open issue against the new partitioning feature in
pg10 and therefore should be addressed now, and others do not. I can see
both sides of that argument.

In any case, thoughts on what to do?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Partitioned tables vs GRANT

[Joe Conway]

Apparently INSERT and SELECT on the parent partitioned table skip normal
acl checks on the partitions. Is that intended behavior?

8<---
test=# create user part_test;
CREATE ROLE
test=#
test=# create table t1 (id int) partition by range ((id % 4));
CREATE TABLE
test=# create table t1_0 partition of t1 for values from (0) to (1);
CREATE TABLE
test=# create table t1_1 partition of t1 for values from (1) to (2);
CREATE TABLE
test=# create table t1_2 partition of t1 for values from (2) to (3);
CREATE TABLE
test=# create table t1_3 partition of t1 for values from (3) to (4);
CREATE TABLE
test=# grant all on TABLE t1 to part_test;
GRANT
test=# set session authorization part_test ;
SET
test=> select current_user;
 current_user
--
 part_test
(1 row)

test=> insert into t1 values(0),(1),(2),(3);
INSERT 0 4
test=> insert into t1_0 values(0);
ERROR:  permission denied for relation t1_0
test=> insert into t1_1 values(1);
ERROR:  permission denied for relation t1_1
test=> insert into t1_2 values(2);
ERROR:  permission denied for relation t1_2
test=> insert into t1_3 values(3);
ERROR:  permission denied for relation t1_3
test=> select * from t1;
 id

  0
  1
  2
  3
(4 rows)

test=> select * from t1_0;
ERROR:  permission denied for relation t1_0
test=> select * from t1_1;
ERROR:  permission denied for relation t1_1
test=> select * from t1_2;
ERROR:  permission denied for relation t1_2
test=> select * from t1_3;
ERROR:  permission denied for relation t1_3
test=> reset session authorization;
RESET
test=# drop table if exists t1;
DROP TABLE
8<---

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/06/2017 12:35 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> Any thoughts on whether 0001a and 0001b ought to be backpatched? I'm
>> thinking not given the lack of past complaints but it might make sense
>> to do.
> 
> I think 0001a absolutely needs to be, because it is fixing what is really
> an ABI violation: sepgsql_needs_fmgr_hook is supposed to return our notion
> of bool, but as things stand it's returning _Bool (which is why the
> compiler is complaining).  Now we might get away with that on most
> hardware, but on platforms where those are different widths, it's possible
> to imagine function-return conventions that would make it fail.
> 
> 0001b seems to only be needed for compilers that aren't smart enough
> to see that tclass won't be referenced for RELKIND_INDEX, so it's
> just cosmetic.

Ok, committed/pushed that way.

I found some missing bits in the 0002 patch -- new version attached.
Will wait on new regression tests before committing, but I expect we'll
have those by end of today and be able to commit the rest tomorrow.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
Add partitioned table support to sepgsql

The new partitioned table capability added a new relkind, namely
RELKIND_PARTITIONED_TABLE. Update sepgsql to treat this new relkind
exactly the same way it does RELKIND_RELATION.

Issue raised by Stephen Frost and initial patch by Mike Palmiotto.
Review by Tom Lane and Robert Haas, and editorializing by me.

Discussion: https://postgr.es/m/flat/623bcaae-112e-ced0-8c22-a84f75ae0c53%40joeconway.com

diff --git a/contrib/sepgsql/dml.c b/contrib/sepgsql/dml.c
index bc17089..b643720 100644
*** a/contrib/sepgsql/dml.c
--- b/contrib/sepgsql/dml.c
*** check_relation_privileges(Oid relOid,
*** 190,195 
--- 190,196 
  	switch (relkind)
  	{
  		case RELKIND_RELATION:
+ 		case RELKIND_PARTITIONED_TABLE:
  			result = sepgsql_avc_check_perms(,
  			 SEPG_CLASS_DB_TABLE,
  			 required,
*** check_relation_privileges(Oid relOid,
*** 225,231 
  	/*
  	 * Only columns owned by relations shall be checked
  	 */
! 	if (relkind != RELKIND_RELATION)
  		return true;
  
  	/*
--- 226,232 
  	/*
  	 * Only columns owned by relations shall be checked
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return true;
  
  	/*
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..6239800 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 787,794 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION ||
! 	relForm->relkind == RELKIND_PARTITIONED_TABLE)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
*** exec_object_restorecon(struct selabel_ha
*** 803,809 
  			case AttributeRelationId:
  attForm = (Form_pg_attribute) GETSTRUCT(tuple);
  
! if (get_rel_relkind(attForm->attrelid) != RELKIND_RELATION)
  	continue;	/* no need to assign security label */
  
  objtype = SELABEL_DB_COLUMN;
--- 812,819 
  			case AttributeRelationId:
  attForm = (Form_pg_attribute) GETSTRUCT(tuple);
  
! if (get_rel_relkind(attForm->attrelid) != RELKIND_RELATION &&
! 	get_rel_relkind(attForm->attrelid) != RELKIND_PARTITIONED_TABLE)
  	continue;	/* no need to assign security label */
  
  objtype = SELABEL_DB_COLUMN;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..59a6d9b 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relations or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/06/2017 08:29 AM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> I'm going to push the attached in a few hours unless there is any
>> additional discussion. As stated above we'll do the regression changes
>> in a separate patch once that is sorted. I used Tom's approach and
>> comment wording for 0001a.
> 
> Looks generally sane, but I noticed a grammatical nitpick:
> 
> -  * Only attributes within regular relation or partition relations have
> +  * Only attributes within regular relations or partition relations have

Good call -- thanks!

Any thoughts on whether 0001a and 0001b ought to be backpatched? I'm
thinking not given the lack of past complaints but it might make sense
to do.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/05/2017 02:29 PM, Mike Palmiotto wrote:
> I'm going to hold the partition table regression changes for a
> separate patch and include some ORDER BY fixes. Will post tomorrow
> 
> In the meantime, attached are the latest and greatest patches.

I'm going to push the attached in a few hours unless there is any
additional discussion. As stated above we'll do the regression changes
in a separate patch once that is sorted. I used Tom's approach and
comment wording for 0001a.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..5e2eba6 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
***
*** 10,15 
--- 10,25 
   */
  #include "postgres.h"
  
+ #include 
+ 
+ /*
+  *  includes , which creates an incompatible
+  * #define for bool.  Get rid of that so we can use our own typedef.
+  * (We don't care if  redefines "true"/"false"; those are close
+  * enough.)
+  */
+ #undef bool
+ 
  #include "access/heapam.h"
  #include "access/htup_details.h"
  #include "access/genam.h"
***
*** 37,44 
  
  #include "sepgsql.h"
  
- #include 
- 
  /*
   * Saved hook entries (if stacked)
   */
--- 47,52 
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..2ea6bfb 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_relation_post_create(Oid relOid)
*** 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16		tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
--- 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16_t	tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
*** sepgsql_relation_drop(Oid relOid)
*** 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
--- 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass = 0;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..4dda82a 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 779,786 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION ||
! 	relForm->relkind == RELKIND_PARTITIONED_TABLE)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..f8689c0 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relation or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 136,144 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_relabel(Oid relOid, At
*** 167,174 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		ereport(ERROR,
  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
   errmsg("cannot set security label on non-regular columns")));
--- 169,177 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		ereport(ERROR,
  (errcode(ERRCODE_WRONG_OBJECT_TYPE),
  	

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/04/2017 09:58 PM, Tom Lane wrote:
> I doubt that works at all, TBH.  What I'd expect to happen with a
> typical compiler is a complaint about redefinition of typedef bool,
> because c.h already declared it and here this fragment is doing
> so again.  It'd make sense to me to do
> 
> + #ifdef bool
> + #undef bool
> + #endif
> 
> to get rid of the macro definition of bool that stdbool.h is
> supposed to provide.  But there should be no reason to declare
> our typedef a second time.

makes sense

> Another issue is whether you won't get compiler complaints about
> redefinition of the "true" and "false" macros.  But those would
> likely only be warnings, not flat-out errors.

I have not been able to generate warnings or errors around "true" and
"false".

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/04/2017 10:02 AM, Joe Conway wrote:
> On 04/04/2017 09:55 AM, Mike Palmiotto wrote:
>> After some discussion off-list, I've rebased and udpated the patches.
>> Please see attached for further review.
> 
> Thanks -- will have another look and test on a machine with selinux
> setup. Robert, did you want me to take responsibility to commit on this
> or just provide review/feedback?

I did some editorializing on these.

In particular I did not like the approach to fixing "warning: ‘tclass’
may be used uninitialized" and ended up just doing it the same as was
done elsewhere in relation.c already (set tclass = 0 in the variable
declaration). Along the way I also changed one instance of tclass from
uint16 to uint16_t for the sake of consistency.

Interestingly we figured out that the warning was present with -Og, but
not present with -O0, -O2, or -O3.

If you want to test, apply 0001a and 0001b before 0002.

Any objections?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..320c098 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
***
*** 10,15 
--- 10,25 
   */
  #include "postgres.h"
  
+ #include 
+ /*
+  * selinux/label.h includes stdbool.h, which redefines bool, so
+  * revert to the postgres definition of bool from c.h
+  */
+ #ifdef bool
+ #undef bool
+ typedef char bool;
+ #endif
+ 
  #include "access/heapam.h"
  #include "access/htup_details.h"
  #include "access/genam.h"
***
*** 37,44 
  
  #include "sepgsql.h"
  
- #include 
- 
  /*
   * Saved hook entries (if stacked)
   */
--- 47,52 
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..2ea6bfb 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_relation_post_create(Oid relOid)
*** 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16		tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
--- 243,249 
  	HeapTuple	tuple;
  	Form_pg_class classForm;
  	ObjectAddress object;
! 	uint16_t	tclass;
  	char	   *scontext;		/* subject */
  	char	   *tcontext;		/* schema */
  	char	   *rcontext;		/* relation */
*** sepgsql_relation_drop(Oid relOid)
*** 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
--- 413,419 
  {
  	ObjectAddress object;
  	char	   *audit_name;
! 	uint16_t	tclass = 0;
  	char		relkind;
  
  	relkind = get_rel_relkind(relOid);
diff --git a/contrib/sepgsql/label.c b/contrib/sepgsql/label.c
index 1a8f884..4dda82a 100644
*** a/contrib/sepgsql/label.c
--- b/contrib/sepgsql/label.c
*** exec_object_restorecon(struct selabel_ha
*** 779,785 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
--- 779,786 
  			case RelationRelationId:
  relForm = (Form_pg_class) GETSTRUCT(tuple);
  
! if (relForm->relkind == RELKIND_RELATION ||
! 	relForm->relkind == RELKIND_PARTITIONED_TABLE)
  	objtype = SELABEL_DB_TABLE;
  else if (relForm->relkind == RELKIND_SEQUENCE)
  	objtype = SELABEL_DB_SEQUENCE;
diff --git a/contrib/sepgsql/relation.c b/contrib/sepgsql/relation.c
index ab98a9b..f8689c0 100644
*** a/contrib/sepgsql/relation.c
--- b/contrib/sepgsql/relation.c
*** sepgsql_attribute_post_create(Oid relOid
*** 54,65 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
  
  	/*
! 	 * Only attributes within regular relation have individual security
! 	 * labels.
  	 */
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 54,66 
  	ObjectAddress object;
  	Form_pg_attribute attForm;
  	StringInfoData audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
  	/*
! 	 * Only attributes within regular relations or partition relations have
! 	 * individual security labels.
  	 */
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
*** sepgsql_attribute_drop(Oid relOid, AttrN
*** 135,142 
  {
  	ObjectAddress object;
  	char	   *audit_name;
  
! 	if (get_rel_relkind(relOid) != RELKIND_RELATION)
  		return;
  
  	/*
--- 136,144 
  {
  	ObjectAddress object;
  	char	   *audit_name;
+ 	char		relkind = get_rel_relkind(relOid);
  
! 	if (relkind != RELKIND_RELATION && relkind != RELKIND_PARTITIONED_TABLE)
  		return;
  
  	/*
***

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/04/2017 09:55 AM, Mike Palmiotto wrote:
> On Tue, Apr 4, 2017 at 10:19 AM, Joe Conway <m...@joeconway.com> wrote:
>> On 04/04/2017 06:45 AM, Robert Haas wrote:
>>> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <m...@joeconway.com> wrote:
>>>>> 0002 looks extremely straightforward, but I wonder if we could get one
>>>>> of the people on this list who knows about sepgsql to have a look?
>>>>> (Stephen Frost, Joe Conway, KaiGai Kohei...)
>>>>
>>>> Will have a look later today.
>>>
>>> I think it is now tomorrow, unless your time zone is someplace very
>>> far away.  :-)
>>
>> OBE -- I have scheduled time in 30 minutes from now, after I have gotten
>> my first cup of coffee ;-)
> 
> After some discussion off-list, I've rebased and udpated the patches.
> Please see attached for further review.

Thanks -- will have another look and test on a machine with selinux
setup. Robert, did you want me to take responsibility to commit on this
or just provide review/feedback?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 04/04/2017 06:45 AM, Robert Haas wrote:
> On Mon, Apr 3, 2017 at 12:02 PM, Joe Conway <m...@joeconway.com> wrote:
>>> 0002 looks extremely straightforward, but I wonder if we could get one
>>> of the people on this list who knows about sepgsql to have a look?
>>> (Stephen Frost, Joe Conway, KaiGai Kohei...)
>>
>> Will have a look later today.
> 
> I think it is now tomorrow, unless your time zone is someplace very
> far away.  :-)

OBE -- I have scheduled time in 30 minutes from now, after I have gotten
my first cup of coffee ;-)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] bumping HASH_VERSION to 3

[Joe Conway]

On 03/31/2017 11:19 AM, Magnus Hagander wrote:
> On Fri, Mar 31, 2017 at 8:17 PM, Robert Haas  > wrote:
> 
> Starting a new thread about this to get more visibility.
> 
> Despite the extensive work that has been done on hash indexes this
> release, we have thus far not made any change to the on-disk format
> that is not nominally backward-compatible.  Commit
> 293e24e507838733aba4748b514536af2d39d7f2 did make a change for new
> hash indexes, but included backward-compatibility code so that old
> indexes would continue to work.  However, I'd like to also commit
> Mithun Cy's patch to expand hash indexes more gradually -- latest
> version in
> 
> http://postgr.es/m/cad__oujd-ibxm91zcqziayftwqjxnfqgmv361v9zke83s6i...@mail.gmail.com
> 
> 
> -- and that's not backward-compatible.
> 
> It would be possible to write code to convert the old metapage format
> to the new metapage format introduced by that patch, and it wouldn't
> be very hard, but I think it would be better to NOT do that, and
> instead force everybody upgrading to v10 to rebuild all of their hash
> indexes.   If we don't do that, then we'll never know whether
> instances of hash index corruption reported against v10 or higher are
> caused by defects in the new code, because there's always the chance
> that the hash index could have been built on a pre-v10 version, got
> corrupted because of the lack of WAL-logging, and then been brought up
> to v10+ via pg_upgrade.  Forcing a reindex in v10 kills three birds
> with one stone:
> 
> - No old, not logged, possibly corrupt hash indexes floating around
> after an upgrade to v10.
> - Can remove the backward-compatibility code added by
> 293e24e507838733aba4748b514536af2d39d7f2 instead of keeping it around
> forever.
> - No need to worry about doing an in-place upgrade of the metapage for
> the above-mentioned patch.
> 
> Thoughts?
> 
> 
> Given the state of hash indexes in <= 9.6, I think this is a reasonable
> tradeoff. Most people won't be using them at all today. Those that do
> will have to "pay" with a REINDEX on upgrade. I think the benefits
> definitely outweigh the cost.
> 
> So +1 for doing it. 

+1


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] partitioned tables and contrib/sepgsql

[Joe Conway]

On 03/31/2017 05:23 PM, Robert Haas wrote:
> On Fri, Mar 31, 2017 at 2:14 PM, Mike Palmiotto
> <mike.palmio...@crunchydata.com> wrote:
>> Attached you will find two patches, which were rebased on master as of
>> 156d388 (applied with `git am --revert [patch file]`). The first gets
>> rid of some pesky compiler warnings and the second implements the
>> sepgsql handling of partitioned tables.
> 
> 0001 has the problem that we have a firm rule against putting any
> #includes whatsoever before "postgres.h".  This stdbool issue has come
> up before, though, and I fear we're going to need to do something
> about it.
> 
> 0002 looks extremely straightforward, but I wonder if we could get one
> of the people on this list who knows about sepgsql to have a look?
> (Stephen Frost, Joe Conway, KaiGai Kohei...)

Will have a look later today.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Schedule and Release Management Team for PG10

[Joe Conway]

On 03/30/2017 10:59 AM, Magnus Hagander wrote:
> On Thu, Mar 30, 2017 at 4:40 PM, Tom Lane  > wrote:
> 
> Robert Haas >
> writes:
> > On Thu, Mar 30, 2017 at 9:41 AM, Bruce Momjian  > wrote:
> >> I propose we go for a week delay in closing the commit fest, and we
> >> decide right now.  Ideally I like to to see delay in one-week 
> increments
> >> _and_ announce that a week before each deadline.
> 
> > Summary of opinions on this thread:
> 
> > - Tom thinks the RMT should consider extending by a day or two.
> 
> FWIW, while I'm sure we had a reason (back at the Ottawa 2016 meeting)
> for limiting the final 'fest to just a month, I'm quite sure we did
> not allow for most of the senior hackers being at a conference during
> the last week of that time.  So I now concur with Bruce's suggestion
> that a week's delay would be suitable.
> 
> 
> +1.

+1


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Faster expression evaluation and targetlist projection.

[Joe Conway]

On 03/25/2017 09:52 PM, Andres Freund wrote:
> On 2017-03-25 20:40:23 -0700, Andres Freund wrote:
>> I blindly tried to fix these, let's hope that works.
> 
> In a second attempt (yes, reading diffs correctly is helpful), this
> resolved the selinux issue. Yeha.

+1!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Re: [COMMITTERS] pgsql: Faster expression evaluation and targetlist projection.

[Joe Conway]

On 03/25/2017 05:21 PM, Andres Freund wrote:
> On March 25, 2017 4:54:08 PM PDT, Joe Conway <m...@joeconway.com> wrote:
>>On 03/25/2017 04:45 PM, Andres Freund wrote:
>>> I think, for starters, seeing regression.diffs from sepgsql would be
>>> useful.  Might already clear up what's the issue.
>>
>>I went looking, and even after a forced run the diff file is gone --
>>does the buildfarm auto-cleanup or something?
> 
> Yes, it does. You'd probably have to run the tests manually.  Do you
> have selinux setup?  I assumed you would, given I seen to recall a
> talk of yours with references to it ;)


Yeah, but those machines are MLS fully constrained, and the sepgsql
regression test specifically needs "targeted" and some other particular
setup. So the easiest box to run this on is the buildfarm animal, but I
also want to do it in a way that doesn't mess up that environment.

I found "keep_error_builds" in build-farm.conf and tried setting to 1
and rerunning in force -- that seems to have worked, so diffs attached.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
*** 
/opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/expected/ddl.out
2017-03-25 17:16:52.707097081 -0700
--- 
/opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/results/ddl.out
 2017-03-25 17:23:17.811479306 -0700
***
*** 187,192 
--- 187,193 
  LOG:  SELinux: allowed { search } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_schema_t:s0 tclass=db_schema 
name="pg_catalog"
  LOG:  SELinux: allowed { search } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_schema_t:s0 tclass=db_schema 
name="pg_catalog"
  LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column 
name="regtest_schema.regtest_table_4.y"
+ LOG:  SELinux: allowed { execute } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_proc_exec_t:s0 tclass=db_procedure 
name="pg_catalog.float8(integer)"
  DROP INDEX regtest_index_tbl4_y;
  LOG:  SELinux: allowed { remove_name } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_schema_t:s0 tclass=db_schema 
name="regtest_schema"
  LOG:  SELinux: allowed { setattr } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_table 
name="regtest_schema.regtest_table_4"

==

*** 
/opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/expected/alter.out
  2017-03-25 17:16:52.707097081 -0700
--- 
/opt/src/pgsql-git/build-farm-root/HEAD/pgsql.build/contrib/sepgsql/results/alter.out
   2017-03-25 17:23:21.280482200 -0700
***
*** 170,177 
--- 170,180 
  LOG:  SELinux: allowed { select } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name="table 
regtest_table column a"
  LOG:  SELinux: allowed { select } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_table 
name="regtest_schema.regtest_table_3"
  LOG:  SELinux: allowed { select } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=unconfined_u:object_r:sepgsql_table_t:s0 tclass=db_column name="table 
regtest_table_3 column x"
+ LOG:  SELinux: allowed { execute } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_proc_exec_t:s0 tclass=db_procedure 
name="pg_catalog.int4eq(integer,integer)"
  ALTER TABLE regtest_table ADD CONSTRAINT test_ck CHECK (b like '%abc%') NOT 
VALID;  -- not supported
  ALTER TABLE regtest_table VALIDATE CONSTRAINT test_ck;  -- not supported
+ LOG:  SELinux: allowed { execute } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_proc_exec_t:s0 tclass=db_procedure 
name="pg_catalog.textlike(pg_catalog.text,pg_catalog.text)"
+ LOG:  SELinux: allowed { execute } 
scontext=unconfined_u:unconfined_r:sepgsql_regtest_superuser_t:s0 
tcontext=system_u:object_r:sepgsql_proc_exec_t:s0 tclass=db_procedure 
name="pg_catalog.textlike(pg_catalog.text,pg_catalog.text)"
  ALTER TABLE regtest_table DROP CONSTRAINT test_ck;  -- not supported
  CREATE TRIGGER regtest_test_trig BEFORE UPDATE ON regtest_table
  FOR EACH ROW EXECUTE PROCEDURE suppre

[HACKERS] Re: [COMMITTERS] pgsql: Faster expression evaluation and targetlist projection.

[Joe Conway]

On 03/25/2017 04:45 PM, Andres Freund wrote:
> On 2017-03-25 16:36:03 -0700, Joe Conway wrote:
>> On 03/25/2017 04:03 PM, Andres Freund wrote:
>> > Hi Joe,
>> > 
>> > 
>> > On 2017-03-25 22:11:37 +, Andres Freund wrote:
>> >> Faster expression evaluation and targetlist projection.
>> > 
>> > Apparently this broke the selinux integration somehow:
>> > https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2017-03-25%2022%3A45%3A01
>> > 
>> > Unfortunately the buildfarm appears to not display regression.diffs -
>> > making this a bit hard to debug remotely.  Any chance you could help me
>> > out with that? I'd rather not setup selinux here...
>> 
>> 
>> Sure. What exactly do you want?
> 
> I think, for starters, seeing regression.diffs from sepgsql would be
> useful.  Might already clear up what's the issue.

I went looking, and even after a forced run the diff file is gone --
does the buildfarm auto-cleanup or something?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

[HACKERS] Re: [COMMITTERS] pgsql: Faster expression evaluation and targetlist projection.

[Joe Conway]

On 03/25/2017 04:03 PM, Andres Freund wrote:
> Hi Joe,
> 
> 
> On 2017-03-25 22:11:37 +, Andres Freund wrote:
>> Faster expression evaluation and targetlist projection.
> 
> Apparently this broke the selinux integration somehow:
> https://buildfarm.postgresql.org/cgi-bin/show_log.pl?nm=rhinoceros=2017-03-25%2022%3A45%3A01
> 
> Unfortunately the buildfarm appears to not display regression.diffs -
> making this a bit hard to debug remotely.  Any chance you could help me
> out with that? I'd rather not setup selinux here...


Sure. What exactly do you want? Should we jump on a call or hangout or
something?

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/16/2017 06:52 AM, Heikki Linnakangas wrote:
> On 03/14/2017 11:14 PM, Tom Lane wrote:
>> In short, I don't think that argument refutes my position that "md5"
>> in pg_hba.conf should be understood as allowing SCRAM passwords too.
> 
> Yeah, let's do that. Here's a patch.
> 
> I had some terminology trouble with the docs. What do you call a user
> that has "md5X" in pgauthid.rolpassword? What about someone with a
> SCRAM verifier? I used the terms "those users that have an MD5 hash set
> in the system catalog", and "users that have set their password as a
> SCRAM verifier", but it feels awkward.

maybe something like:
"those users with an MD5 hashed password"
"those users with a SCRAM verifier hash"

> The behavior when a user doesn't exist, or doesn't have a valid
> password, is a bit subtle. Previously, with 'md5' authentication, we
> would send the client an MD5 challenge, and fail with "invalid password"
> error after receiving the response. And with 'scram' authentication, we
> would perform a dummy authentication exchange, with a made-up salt. This
> is to avoid revealing to an unauthenticated client whether or not the
> user existed.
> 
> With this patch, the dummy authentication logic for 'md5' is a bit more
> complicated. I made it look at the password_encryption GUC, and send the
> client a dummy MD5 or SCRAM challenge based on that. The idea is that
> most users presumably have a password of that type, so we use that
> method for the dummy authentication, to make it look as "normal" as
> possible. It's not perfect, if password_encryption is set to 'scram',
> and you probe for a user that has an MD5 password set, you can tell that
> it's a valid user from the fact that the server sends an MD5 challenge.

Presumably if you are unauthenticated you don't have any way to know
what password_encryption is set to, so this seems pretty reasonable.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Changing references of password encryption to hashing

[Joe Conway]

On 03/16/2017 06:19 AM, Robert Haas wrote:
> On Mon, Mar 13, 2017 at 4:48 AM, Craig Ringer  wrote:
>> So I'm in favour of fixing the docs but I'm not keen on changing the
>> SQL syntax in a way that just kind of papers over part of the
>> problems.
> 
> I agree.  I think that trying to design new SQL syntax at this point
> is unlikely to be a good idea - we're just about out of time here, and
> some people who might care about this are busy on other things, and
> the deadline for patches that do new things has long since passed.
> But I like the idea of trying to improve the documentation.


Agreed. I think the documentation fixes definitely should be done, but
understand that the grammar is a longer term issue with backward
compatibility implications. Acknowledging the problem is the first step ;-)

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/15/2017 08:48 AM, Michael Paquier wrote:
> I have been hacking my way through this thing, and making
> scram_build_verifier is requiring a bit more refactoring than I
> thought first:
> - stored and server keys are hex-encoded using a backend-only routine.
> I think that those should be instead base64-encoded using what has
> already been moved in src/common/.

Or possibly make the hex routines available in the front end as well
instead?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/14/2017 08:40 AM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> On 03/14/2017 03:15 AM, Heikki Linnakangas wrote:
>>> It would be a lot more sensible, if there was a way to specify in
>>> pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but
>>> perhaps we should try to cram that in, after all.
> 
>> I was also thinking about that. Basically a primary method and a
>> fallback. If that were the case, a gradual transition could happen, and
>> if we want \password to enforce best practice it would be ok.
> 
> Why exactly would anyone want "md5 only"?  I should think that "scram
> only" is a sensible pg_hba setting, if the DBA feels that md5 is too
> insecure, but I do not see the point of "md5 only" in 2017.  I think
> we should just start interpreting that as "md5 or better".

That certainly would work for me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/14/2017 03:15 AM, Heikki Linnakangas wrote:
> On 03/14/2017 04:47 AM, Tom Lane wrote:
>> Robert Haas  writes:
>>> I'm not talking about changing the default, just having it be possible
>>> to use \password with the new system as it was with the old, whatever
>>> exactly we think that means.
>>
>> Seems to me the intended behavior of \password is to use the best
>> available practice.  So my guess is that it ought to use SCRAM when
>> talking to a >= 10.0 server.  What the previous password was ought
>> to be irrelevant, even if it could find that out which it shouldn't
>> be able to IMO.
> 
> If the server isn't set up to do SCRAM authentication, i.e. there are no
> "scram" entries in pg_hba.conf, and you set yourself a SCRAM verifier,
> you have just locked yourself out of the system. I think that's a
> non-starter. There needs to be some more intelligence in the decision.


Yes, this was exactly my concern.

> It would be a lot more sensible, if there was a way to specify in
> pg_hba.conf, "scram-or-md5". We punted on that for PostgreSQL 10, but
> perhaps we should try to cram that in, after all.


I was also thinking about that. Basically a primary method and a
fallback. If that were the case, a gradual transition could happen, and
if we want \password to enforce best practice it would be ok.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/12/2017 08:10 PM, Michael Paquier wrote:
> OK, so what about doing the following then:
> 1) Create pg_role_password_type(oid), taking a role OID in input and
> returning the type.

That version would make sense for administrative use. I still think we
might want a version of this that takes no argument, works on the
current_user, and is executable by anyone.

> 2) Extend PQencryptPassword with a method, and document. Plaintext is 
> forbidden.

Check, although if "plain" were allowed as a method for the sake of
consistency/completeness the function could just immediately return the
argument.

> 3) Extend \password in psql with a similar -method=scram|md5 argument,
> and forbid the use of "plain" format.

Not sure why we would forbid "plain" here unless we remove it entirely
elsewhere.

> After thinking about it, extending PQencryptPassword() would lead to
> future breakage, which makes it clear to not fall into the trap of
> having password_encryption set to scram in the server's as well as in
> pg_hba.conf and PQencryptPassword() enforcing things to md5.

I'm not grokking this statement

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/11/2017 11:07 PM, Michael Paquier wrote:
> Having a RLS on pg_authid to allow a user to look at its own password
> type is an idea.

Given that that is not likely at this stage of the dev cycle, what about
a special purpose SQL function that returns the password type for the
current user? Or is it a security issue of some sort to allow a user to
know their own password type?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/11/2017 03:15 PM, Michael Paquier wrote:
> Yeah but it can be called as well while the application is calling
> PQgetResult() and still looping until it gets a NULL result. Not sure
> if this is a use-case to worry about, but sending a query to the
> server in PQencryptPassword() could as well break some applications.

I was suggesting sending the query outside of PQencryptPassword() in
order to determine what method should be passed as a new argument to
PQencryptPassword().

> PQencryptPassword() is used for CREATE/ALTER ROLE commands, so
> actually wouldn't it make sense to just switch PQencryptPassword to
> handle SCRAM if at some point we decide to switch the default from md5
> to scram? So many questions.

As long as we support more than one method it would seem to me we need a
way to determine which one we want to use and not only default it, don't
we? Apologies if this has already been discussed -- I was not able to
follow the lengthy threads on SCRAM in any detail.

>> I guess a related problem might be, do we have a SQL visible way to
>> determine what method is used by the current password for a given role?
> 
> Nope. We are simply looking at a function doing a lookup at pg_authid
> and then use get_password_type() to check which type of verifier is
> used... Or have the type of verifier as a new column of pg_authid,
> information that could be made visible to any users with column
> privileges.

What happens if the user does not have privs for pg_authid? E.g. if I
want to change my own password what happens if the default is one
method, and my password uses the other -- now I cannot change my own
password using \password?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/11/2017 02:21 PM, Michael Paquier wrote:
> On Sun, Mar 12, 2017 at 5:35 AM, Joe Conway <m...@joeconway.com> wrote:
>> So if the password is not already set, \password uses
>> password_encryption to determine which format to use, and if the
>> password is already set, then the current method is assumed.
> 
> Yeah, the problem here being that this routine does not need a live
> connection to work, and we surely don't want to make that mandatory,
> that's why I am suggesting something like the above. Another approach
> would be to switch to SCRAM once password_encryption does this switch
> as well... There is no perfect scenario here.

You might extend PQencryptPassword() to take a method. Or create a new
function that does? Certainly psql has a connection available to run the
ALTER ROLE command that it crafts.

I guess a related problem might be, do we have a SQL visible way to
determine what method is used by the current password for a given role?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Changing references of password encryption to hashing

[Joe Conway]

On 03/09/2017 06:16 PM, Michael Paquier wrote:
> As discussed here:
> https://www.postgresql.org/message-id/98cafcd0-5557-0bdf-4837-0f2b7782d...@joeconway.com
> We are using in documentation and code comments "encryption" to define
> what actually is hashing, which is confusing.
> 
> Attached is a patch for HEAD to change the documentation to match hashing.
> 
> There are a couple of things I noticed on the way:
> 1) There is the user-visible PQencryptPassword in libpq, which
> actually hashes the password, and not encrypts it :)
> 2) There is as well pg_md5_encrypt() in the code, as well as there is
> pg_md5_hash(). Those may be better if renamed, at least I would think
> that pg_md5_encrypt should be pg_md5_password_hash, because a salt is
> used as well, and the routine is dedicated to work on passwords.
> Thoughts?
> 3) createuser also has --encrypt and --unencrypted, perhaps those
> should be renamed? Honestly I don't really think that this is worth a
> breakage and the option names match with the SQL commands.

My opinion is that the user visible aspects of this should be deprecated
and correct syntax provided. But perhaps that is overkill.

8<
key and server key, respectively, in hexadecimal format. A password that
-   does not follow either of those formats is assumed to be unencrypted.
+   does not follow either of those formats is assumed to be in plain
format,
+   non-hashed.
   
  
8<
I think here, instead of "in plain format, non-hashed" it is ok to just
say "cleartext" or maybe "plaintext". Whichever is picked, it should be
used consistently.

8<
  
   
-   To prevent unencrypted passwords from being sent across the network,
+   To prevent non-hashed passwords from being sent across the network,
8<
same here "non-hashed" ==> "cleartext"

8<
   
-   Caution must be exercised when specifying an unencrypted password
+   Caution must be exercised when specifying a non-hashed password
8<
and here

...etc...

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [bug fix] dblink leaks unnamed connections

[Joe Conway]

On 03/09/2017 08:31 AM, Joe Conway wrote:
> On 03/09/2017 07:54 AM, Tom Lane wrote:
>> Fujii Masao <masao.fu...@gmail.com> writes:
>>> On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
>>> <tsunakawa.ta...@jp.fujitsu.com> wrote:
>>>> dblink fails to close the unnamed connection as follows when a new unnamed 
>>>> connection is requested.  The attached patch fixes this.
>> 
>>> This issue was reported about ten years ago and added as TODO item.
>>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php
>> 
>>> I agree that this is a bug, and am tempted to back-patch to all the 
>>> supported
>>> versions. But it had not been fixed in many years since the first report of
>>> the issue. So I'm not sure if it's ok to just treat this as a bug right now 
>>> and
>>> back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
>>> more opinions about this.
>> 
>> It looks to me like the issue simply fell through the cracks because Joe
>> wasn't excited about fixing it.  Now that we have a second complaint,
>> I think it's worth treating as a bug and back-patching.
>> 
>> (I've not read this particular patch and am not expressing an opinion
>> whether it's correct.)
> 
> Ok, will take another look.

I pushed a fix to all supported branches.

Thanks,

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] scram and \password

[Joe Conway]

On 03/10/2017 02:43 PM, Michael Paquier wrote:
> On Sat, Mar 11, 2017 at 2:53 AM, Jeff Janes  wrote:
>> Should the \password tool in psql inspect password_encryption and act on it
>> being 'scram'?
> 
> Not sure if it is wise to change the default fot this release.
> 
>> I didn't see this issue discussed, but the ability to search the archives
>> for backslashes is rather limited.
> 
> I'll save you some time: it has not been discussed. Nor has
> PQencryptPassword been mentioned. Changing to SCRAM is not that
> complicated, just call scram_build_verifier() and you are good to go.
> 
> Instead of changing the default, I think that we should take this
> occasion to rename PQencryptPassword to something like
> PQhashPassword(), and extend it with a method argument to support both
> md5 and scram. PQencryptPassword could also be marked as deprecated,
> or let as-is for some time. For \password, we could have another
> meta-command but that sounds grotty, or just extend \password with a
> --method argument. Switching the default could happen in another
> release.
> 
> A patch among those lines would be a simple, do people feel that this
> should be part of PG 10?

Seems like it should work in an analogous way to CREATE/ALTER ROLE.
According to the docs:

8<
ENCRYPTED
UNENCRYPTED

These key words control whether the password is stored encrypted in
the system catalogs. (If neither is specified, the default behavior is
determined by the configuration parameter password_encryption.) If the
presented password string is already in MD5-encrypted or SCRAM-encrypted
format, then it is stored encrypted as-is, regardless of whether
ENCRYPTED or UNENCRYPTED is specified (since the system cannot decrypt
the specified encrypted password string). This allows reloading of
encrypted passwords during dump/restore.
8<

So if the password is not already set, \password uses
password_encryption to determine which format to use, and if the
password is already set, then the current method is assumed.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Upgrading postmaster's log messages about bind/listen errors

[Joe Conway]

On 03/09/2017 12:27 PM, Tom Lane wrote:
> Over in
> https://www.postgresql.org/message-id/flat/201703072317.01345.john.iliffe%40iliffe.ca
> we spent quite a lot of effort to diagnose what turned out to be a simple
> networking misconfiguration.  It would probably have taken a lot less
> effort if the postmaster were more forthcoming about exactly what address
> it's trying to bind to.  I seem to recall having wanted to include that
> info in the messages many years ago, but at the time we lacked any
> reasonably-portable way to decode a struct addrinfo.  Now we have
> pg_getnameinfo_all(), so PFA a patch to include the specific address in
> any complaint about failures in the socket/bind/listen sequence.
> 
> For good measure I also added a DEBUG1 log message reporting successful
> binding to a port.  I'm not sure if there's an argument for putting this
> out at LOG level (i.e. by default) --- any thoughts about that?

+1 for making it LOG instead of DEBUG1

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] [bug fix] dblink leaks unnamed connections

[Joe Conway]

On 03/09/2017 07:54 AM, Tom Lane wrote:
> Fujii Masao  writes:
>> On Wed, Mar 8, 2017 at 3:52 PM, Tsunakawa, Takayuki
>>  wrote:
>>> dblink fails to close the unnamed connection as follows when a new unnamed 
>>> connection is requested.  The attached patch fixes this.
> 
>> This issue was reported about ten years ago and added as TODO item.
>> http://archives.postgresql.org/pgsql-hackers/2007-10/msg00895.php
> 
>> I agree that this is a bug, and am tempted to back-patch to all the supported
>> versions. But it had not been fixed in many years since the first report of
>> the issue. So I'm not sure if it's ok to just treat this as a bug right now 
>> and
>> back-patch. Or we should fix this only in HEAD? Anyway I'd like to hear
>> more opinions about this.
> 
> It looks to me like the issue simply fell through the cracks because Joe
> wasn't excited about fixing it.  Now that we have a second complaint,
> I think it's worth treating as a bug and back-patching.
> 
> (I've not read this particular patch and am not expressing an opinion
> whether it's correct.)

Ok, will take another look.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

[Joe Conway]

On 03/09/2017 06:34 AM, Robert Haas wrote:
> On Thu, Mar 9, 2017 at 7:59 AM, Michael Paquier
>  wrote:
>> Yes, I agree with that for MD5, and after looking around I can see
>> (like here http://prosody.im/doc/plain_or_hashed) as well that
>> SCRAM-hashed is used. Now, there are as well references to the salt,
>> like in protocol.sgml:
>> "The salt to use when encrypting the password."
>> Joe, do you think that in this case using the term "hashing" would be
>> more appropriate? I would think so as we use it to hash the password.
> 
> I'm not Joe, but I think that would be more appropriate.


I am Joe and I agree ;-)


-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] CREATE/ALTER ROLE PASSWORD ('value' USING 'method')

[Joe Conway]

On 03/07/2017 08:29 PM, Tom Lane wrote:
> Michael Paquier  writes:
>> here is a separate thread dedicated to the following extension for
>> CREATE/ALTER ROLE: PASSWORD ('value' USING 'method').
> 
> The parentheses seem weird ... do we really need those?

+1

> +If you do not plan to use password authentication you can omit this
> +option. The methods supported are md5 to enforce a 
> password
> +to be MD5-encrypted, scram for a SCRAM-encrypted password
> +and plain for an unencrypted password.  If the password

Can we please stop calling this encryption? What is being done is a form
of cryptographic hashing, not encryption.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] New Committer - Andrew Gierth

[Joe Conway]

On 02/28/2017 10:22 AM, Stephen Frost wrote:
> Greetings!
> 
> The PostgreSQL committers would like to welcome Andrew Gierth as a new
> committer for the PostgreSQL project.
> 
> Andrew - welcome!

Congratulations!

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Retiring from the Core Team

[Joe Conway]

On 01/11/2017 04:29 PM, Josh Berkus wrote:
> For that reason, as of today, I am stepping down from the PostgreSQL
> Core Team.



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

End of an era! Thank you for your many efforts for the community, past,
present, and future. Hope we'll still see you at various events during
the year.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] ALTER SYSTEM for pg_hba.conf

[Joe Conway]

On 01/05/2017 08:27 AM, Robert Haas wrote:
> There's also the question of whether opening up the ability to do
> this sort of thing from the SQL level is a security hazard,

It unquestionably is.

> but we've already gone fairly far down the path of assuming that
> there's not a tremendous amount of privilege separation between the
> operating system user account and the database superuser,

I think this is a very bad assumption.

> so maybe the answer is that as things stand it's not expanding the 
> vulnerability surface very much.

Perhaps as things currently stand this is true.

> One thing I'm kind of happy about is that, as far as I can see, there
> hasn't been much backlash against the existing ALTER SYSTEM, either
> from a security point of view or a user-confusion point of view.

Possibly only because there are workarounds possible using hooks and
extension code. Personally I think we should have an official way to
disable ALTER SYSTEM and I would like the same for pg_hba.conf related
functionality.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: session server side variables

[Joe Conway]

On 01/04/2017 04:36 PM, Craig Ringer wrote:
> On 5 January 2017 at 08:35, Craig Ringer  wrote:
>> On 5 January 2017 at 01:49, Fabien COELHO  wrote:

>>> Good. So we seem to agree that GUCS are transactional?
>>
>> No. We don't agree. They aren't.
> 
> Uh. I take that back.
> 
> craig=> SET x.s = 'x';
> SET
> craig=> BEGIN;
> BEGIN
> craig=> SET x.s = 'y';
> SET
> craig=> ROLLBACK;
> ROLLBACK
> craig=> SHOW x.s;
>  x.s
> -
>  x
> (1 row)
> 
> 
> I'm surprised, I never knew this.


(I have not been able to keep up with the shear volume on this thread,
 but this caught my eye...)

Yeah -- I found it surprising when I first discovered it too. My opinion
is that the design for variables should not behave this way.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Compiler warnings

[Joe Conway]

On 01/02/2017 10:55 AM, Joe Conway wrote:
> On the 9.2 and 9.3 branches I see two warnings:

> This one once:
> ---
> plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
> 
> And this one once per bison file:
> ---
> gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
> [-Wdeprecated]
>  %name-prefix="base_yy"
>  ^

> Starting in 9.5 I only get the plancache.c warning and this one:
> ---
> lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
> function [-Wmaybe-uninitialized]
>   if (mode == LW_EXCLUSIVE)

Peter Eisentraut's Bison deprecation warnings patch (per Tom's reply
nearby in this thread) back-patched to 9.2 and 9.3 branches.
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=55fb759ab3e7543a6be72a35e6b6961455c5b393

Stephen Frosts's plancache.c back-patched to 9.6 through 9.2
and lwlock.c back-patched 9.6 through 9.5:
https://git.postgresql.org/gitweb/?p=postgresql.git;a=commit;h=d97b14ddab2059e1d73c0cd17f26bac4ef13e682

> For the sake of completeness, in 9.4. I get the plancache.c warning and
> this one:
> ---
> basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
> [-Wunused-but-set-variable]
>   int   wait_result;

This one is no longer seen -- I must have neglected to pull before
making that comment.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Compiler warnings

[Joe Conway]

On 01/02/2017 11:09 AM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> If there is agreement on fixing these warnings, other than the bison
>> generated warning, I would be happy to do it. I'd also be happy to look
>> for a fix the bison warning as well if desired, but that should be
>> handled separately I would think.
> 
> The bison issue is discussed in
> https://www.postgresql.org/message-id/flat/E1WpjkB-0003zA-N4%40gemulon.postgresql.org

Ah, thanks. I vaguely remember that thread now.

Looks like there was some consensus for applying Peter's patch with the
addition of a comment, but apparently that never happened. Would we
still consider that for 9.2 and 9.3 branches?

Any thoughts on fixing the other warnings?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Compiler warnings

[Joe Conway]

On 01/02/2017 10:18 AM, Robert Haas wrote:
> On Thu, Dec 29, 2016 at 10:47 AM, Joe Conway <m...@joeconway.com> wrote:
>> Shouldn't this be back-patched? The plancache warning goes back through
>> 9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).
> 
> Warnings are going to be different for each individual developer, but
> I am cautiously in favor making more of an effort to fix back-branch
> warnings provided that it doesn't generate too much code churn.  For
> example, if your toolchain generates only these two warnings on 9.2
> then, sure, let's back-port these two fixes; making things
> warning-clean is great.  But if there are dozens or hundreds of
> warnings currently, fixing only a handful of those warnings probably
> isn't valuable, and fixing all of them is probably a little more risk
> than we necessarily want to take.  Someone could goof and make a bug.
> On my MacBook Pro with my toolchain, we're warning-clean back to 9.3
> or 9.4, and before that there are some problems -- most annoyingly the
> fact that 73b416b2e41237b657d29d8f42a4bb34bf700928 couldn't be easily
> backported to older branches.  I don't think it would be crazy to try
> to get all of the warnings I see fixed up and it would be convenient
> for me, but I haven't been willing to do the work, either.
> 

FWIW, I'm running Mint 18 which is based on Unbuntu 16.04 I believe. On
the 9.2 and 9.3 branches I see two warnings:

This one once:
---
plancache.c:1197:9: warning: ‘plan’ may be used uninitialized in this
function [-Wmaybe-uninitialized]

And this one once per bison file:
---
gram.y:169.1-13: warning: deprecated directive, use ‘%name-prefix’
[-Wdeprecated]
 %name-prefix="base_yy"
 ^

The plancache.c fix in Stephen's patch was really simple: initialize
plan = NULL and add an assert. To me that seems like something we should
definitely back-patch.

On the other hand, seems like we have had bison related warnings of one
kind or another since I first got involved with Postgres. So while it
would be nice to make that one go away, it somehow bothers me less. It
also goes away starting in 9.4 anyway.


Starting in 9.5 I only get the plancache.c warning and this one:
---
lwlock.c:1555:5: warning: ‘mode’ may be used uninitialized in this
function [-Wmaybe-uninitialized]
  if (mode == LW_EXCLUSIVE)
 ^

That is the other warning fixed in Stephens commit to master.

For the sake of completeness, in 9.4. I get the plancache.c warning and
this one:
---
basebackup.c:1284:6: warning: variable ‘wait_result’ set but not used
[-Wunused-but-set-variable]
  int   wait_result;
  ^

Seems like that should be easily fixed.

If there is agreement on fixing these warnings, other than the bison
generated warning, I would be happy to do it. I'd also be happy to look
for a fix the bison warning as well if desired, but that should be
handled separately I would think.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Compiler warnings

[Joe Conway]

On 12/06/2016 01:59 PM, Robert Haas wrote:
> On Tue, Dec 6, 2016 at 3:46 PM, Stephen Frost  wrote:
>> Good thought, thanks!
>>
>> Updated patch attached with that change and I also added an Assert() to
>> GetCachedPlan(), in case that code gets whacked around later and somehow
>> we end up falling through without actually setting *plan.
>>
>> Thoughts?
> 
> wfm
> 

Shouldn't this be back-patched? The plancache warning goes back through
9.2 (at least) and the lwlocks warning through 9.5 (or maybe it was 9.4).

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: session server side variables

[Joe Conway]

On 12/28/2016 10:17 AM, Jim Nasby wrote:
> Then IMHO what needs to happen is to have a discussion on actual syntax
> instead of calling into question the value of the feature. Following
> this thread has been very painful because the communications have not
> been very clear. Focus on grammar would probably be a big improvement in
> that regard.

+1

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Incautious handling of overlength identifiers

[Joe Conway]

On 12/23/2016 12:44 PM, Tom Lane wrote:
> I wrote:
>> So what to do?  We could run around and fix these individual cases
>> and call it good, but if we do, I will bet a very fine dinner that
>> more such errors will sneak in before long.  Seems like we need a
>> coding convention that discourages just randomly treating a C string
>> as a valid value of type NAME.  Not sure how to get there though.
> 
> An alternative worth considering, especially for the back branches,
> is simply to remove the Assert in hashname().  That would give us
> the behavior that non-developers see anyway, which is that these
> functions always fail to match overlength names, whether or not
> the names would have matched after truncation.  Trying to apply
> truncation more consistently could be left as an improvement
> project for later.

That sounds reasonable to me.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] proposal: session server side variables

[Joe Conway]

On 12/23/2016 08:20 AM, Pavel Stehule wrote:
> 2016-12-23 16:27 GMT+01:00 Fabien COELHO:
>> I have often wished I had such a feature, psql client side :-variables are
>> just awful raw text things.

Agreed.

>> A few comments, mostly about the design:
>>
>> 1. persistent objects with temporal unshared typed content. The life of
>>> content should be limited by session or by transaction. The content is
>>> initialized to default (when it is defined) or to NULL when variable is
>>> first accessed in variable' time scope (session, transaction).
>>>
>>> CREATE VARIABLE [schema.]variable type [DEFAULT default_value]
>>> [TRANSACTION|SESION SCOPE]


I haven't looked, but I take it the SQL standard is silent on the issue
of variables?

> I really would to use pg_class as base for metadata of variables -
> conflicts are not possible. I can reuse safe GRANT/REVOKE mechanism ..

That would be very useful.

>> In the long term, What would be the possible scopes?
>>
>> TRANSACTION, SESSION, PERSISTANT ?
>>
>> Would some scopes orthogonal (eg SHARED between sessions for a USER in a
>> DATABASE, SHARED at the cluster level?).
> 
> I have a plan to support TRANSACTION and SESSION scope. Persistent or
> shared scope needs much more complex rules, and some specialized extensions
> will be better.


I can see where persistent variables would be very useful though.


>> 2. accessed with respecting access rights:
>>>
>>> GRANT SELECT|UPDATE|ALL ON VARIABLE variable TO role
>>> REVOKE SELECT|UPDATE|ALL ON VARIABLE variable FROM role
>>
>> At least for transaction and session scopes it does not make sense that
>> they would be accessible outside the session/transaction, so grant/revoke
>> do not seem necessary?
> 
> It is necessary - and I think so it is fundamental feature - any other
> features can be more or less replaced by extensions, but this one cannot or
> not simply  - you have to protect content against some users - some
> cookies, ids have to be protected. It can be used well with RLS.

How would this work for transaction and session scopes though? What
would be the point -- no other access is possible other than what
happens in the session. Do you envision something like

 CREATE VARIABLE foo ...;
 GRANT SELECT ON VARIABLE foo TO bob;
 SET ROLE bob;

?

>> 3. accessed/updated with special function "getvar", "setvar":
>>>
>>> FUNCTION getvar(regclass) RETURNS type
>>> FUNCTION setvar(regclass, type) RETURNS void
>>>
>>
>> From an aesthetical point of view, I do not like that much.
>>
>> If you use CREATE & DROP, then logically you should use ALTER:
>>
>>   CREATE VARIABLE @name TEXT DEFAULT 'calvin';
>> CREATE VARIABLE @name TEXT = 'calvin';
>>   ALTER VARIABLE @name SET VALUE TO 'hobbes';
>> ALTER VARIABLE @name = 'hoobes';
>>   DROP VARIABLE @name;

Makes sense.

>> Maybe "SET" could be an option as well, but it is less logical:
>>
>>   SET @name = 'susie';
>>
>> But then "SET @..." would just be a shortcut for ALTER VARIABLE.

Maybe. Not sure I like that.

> I would to use a SET statement too. But it is used for another target now.
> Using ALTER in this content looks strange to me. It is used for changing
> metadata not a value.
> 
> Next step can be support of SQL statements
> 
> With SQL support you can do
> 
> SELECT varname;

+1

> SELECT * FROM compositevarname;

+1

> UPDATE varname SET value TO xxx;
> UPDATE compositevarname SET field TO xxx;

These need more thought I think.

>> Also a nicer way to reference them would be great, like SQL server.
>>
>>   SELECT * FROM SomeTable WHERE name = @name;
>>
>> A function may be called behind the scene, I'm just arguing about the
>> syntax here...
>>
>> Important question, what nice syntax to assign the result of a query to a
>> variable? Maybe it could be:
>>
>>   SET @name = query-returning-one-row; -- hmmm
>>   SET @name FROM query-returning-one-row; -- maybe better
>>
>> Or:
>>
>>   ALTER VARIABLE @name WITH one-row-query;
>>
>> Special variables could allow to get the number of rows modified by the
>> last option, like in PL/pgSQL but at the SQL level?

I think the SET syntax is growing on me, but I suspect there may be push
back on overloading that syntax.

>> 4. non transactional  - the metadata are transactional, but the content is
>>> not.
>>>
>>
>> Hmmm... Do you mean:
>>
>> CREATE VARIABLE foo INT DEFAULT 1 SCOPE SESSION;
>> BEGIN;
>>   SET @foo = 2;
>> ROLLBACK;
>>
>> Then @foo is 2 despite the roolback? Yuk!

Agreed

> This is similar to sequences.

I don't see how variables really have anything to do with sequences.

> If you need transactional content - then you should to use tables.

I definitely have use-cases where transactional variables would be useful.


>> I think that if the implementation is based on some system table for
>> storage, then you could get the transaction properties for free, and it
>> seems more logical to do so:
>>
>> CREATE TEMPORARY TABLE pg_session_variables(name TEXT PRIMARY KEY, value
>> TEXT, oidtype, ...);
>>

Re: [HACKERS] RLS related docs

[Joe Conway]

On 09/15/2016 02:34 PM, Joe Conway wrote:
> On 09/15/2016 01:33 PM, Robert Haas wrote:
>> On Sun, Aug 28, 2016 at 4:23 PM, Joe Conway <m...@joeconway.com> wrote:
>>>>> For COPY, I think perhaps it would be more logical to put the new note
>>>>> immediately after the third note which describes the privileges
>>>>> required, since it's kind of related, and then we can talk about the
>>>>> RLS policies required, e.g.:
>>>>>
>>>>> If row-level security is enabled for the table, COPY table TO is
>>>>> internally converted to COPY (SELECT * FROM table) TO, and the
>>>>> relevant security policies are applied. Currently, COPY FROM is not
>>>>> supported for tables with row-level security.
>>>>
>>>> This sounds better than what I had, so I will do it that way.
>>>
>>> Apologies for the delay, but new patch attached. Assuming no more
>>> comments, will commit this, backpatched to 9.5, in a day or two.
>>
>> I don't think this was ever committed, but my comment is that it seems
>> to be exposing rather more of the implementation than is probably
>> wise.  Can't we say that SELECT policies will apply rather than saying
>> that it is internally converted to a SELECT?

Committed that way, backpatched to 9.5.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Getting rid of "unknown error" in dblink and postgres_fdw

[Joe Conway]

On 12/22/2016 06:55 AM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> On 12/21/2016 09:20 PM, Tom Lane wrote:
>>> I see that you need to pass the PGconn into dblink_res_error() now, but
>>> what's the point of the new "bool fail" parameter?
> 
>> That part isn't new -- we added it sometime prior to 9.2:
> 
> Oh!  I misread the patch --- something about an unluckily-placed line
> wrap and not looking very closely :-(.  Yeah, it's fine as is.
> Sorry for the noise.

Thanks -- committed/backpatched to 9.2

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] dblink get_connect_string() passes FDW option "updatable" to the connect string, connection fails.

[Joe Conway]

On 12/18/2016 02:47 PM, Corey Huinker wrote:
> On Sun, Dec 18, 2016 at 4:57 PM, Michael Paquier wrote:
>> On Mon, Dec 19, 2016 at 6:48 AM, Joe Conway wrote:
>>> Maybe if "CREATE FOREIGN DATA WRAPPER" had a way to specify that the FDW
>>> supports a libpq connection it would make sense to allows other FDWs
>>> with this attribute, but since there is none the current state strikes
>>> me as a bad idea.
>>> Thoughts?

>> libpq is proper to the implementation of the FDW, not the wrapper on
>> top of it, so using in the CREATE FDW command a way to do the
>> decision-making that does not look right to me. Filtering things at
>> connection attempt is a better solution.

> The only benefit I see would be giving the user a slightly more clear
> error message like ('dblink doesn't work with %', 'mysql') instead of
> the error about the failure of the connect string generated by the
> options that did overlap. 

Ok, I committed Corey's patch with only minor whitespace changes.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Getting rid of "unknown error" in dblink and postgres_fdw

[Joe Conway]

On 12/21/2016 09:20 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> On 12/21/2016 04:22 PM, Tom Lane wrote:
>>> In short, yes, please copy that bit into dblink.
> 
>> The attached should do the trick I think.
> 
> I see that you need to pass the PGconn into dblink_res_error() now, but
> what's the point of the new "bool fail" parameter?

That part isn't new -- we added it sometime prior to 9.2:

8<--
if (fail)
level = ERROR;
else
level = NOTICE;
8<--

It allows dblink to throw a NOTICE on remote errors rather than an
actual ERROR, e.g. for an autonomous transaction.

From the docs (9.2 in this case)
8<--
fail_on_error

If true (the default when omitted) then an error thrown on the
remote side of the connection causes an error to also be thrown locally.
If false, the remote error is locally reported as a NOTICE, and the
function returns no rows.
8<--

>> You think it is reasonable to backpatch this part too?
> 
> Yes, definitely.

Ok, will do.

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development



signature.asc
Description: OpenPGP digital signature

Re: [HACKERS] Getting rid of "unknown error" in dblink and postgres_fdw

[Joe Conway]

On 12/21/2016 04:22 PM, Tom Lane wrote:
> Joe Conway <m...@joeconway.com> writes:
>> I did notice that postgres_fdw has the following stanza that I don't see
>> in dblink:
> 
>> 8<--
>> /*
>>  * If we don't get a message from the PGresult, try the PGconn.  This
>>  * is needed because for connection-level failures, PQexec may just
>>  * return NULL, not a PGresult at all.
>>  */
>> if (message_primary == NULL)
>>  message_primary = PQerrorMessage(conn);
>> 8<--
> 
>> I wonder if the original issue on pgsql-bugs was a connection-level
>> failure rather than OOM? Seems like dblink ought to do the same.
> 
> Oooh ... I had thought that code was in both, which was why I was having
> a hard time explaining the OP's failure.  But I see you're right,
> which provides a very straightforward explanation for the report.
> I believe that if libpq is unable to malloc a PGresult, it will return
> NULL but put an "out of memory" message into the PGconn's error buffer.
> I had supposed that we'd capture and report the latter, but as the
> dblink code stands, it won't.
> 
> In short, yes, please copy that bit into dblink.

The attached should do the trick I think. You think it is reasonable to
backpatch this part too?

Joe

-- 
Crunchy Data - http://crunchydata.com
PostgreSQL Support for Secure Enterprises
Consulting, Training, & Open Source Development
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index ee45cd2..db3085a 100644
*** a/contrib/dblink/dblink.c
--- b/contrib/dblink/dblink.c
*** static Relation get_rel_from_relname(tex
*** 112,118 
  static char *generate_relation_name(Relation rel);
  static void dblink_connstr_check(const char *connstr);
  static void dblink_security_check(PGconn *conn, remoteConn *rconn);
! static void dblink_res_error(const char *conname, PGresult *res, const char *dblink_context_msg, bool fail);
  static char *get_connect_string(const char *servername);
  static char *escape_param_str(const char *from);
  static void validate_pkattnums(Relation rel,
--- 112,119 
  static char *generate_relation_name(Relation rel);
  static void dblink_connstr_check(const char *connstr);
  static void dblink_security_check(PGconn *conn, remoteConn *rconn);
! static void dblink_res_error(PGconn *conn, const char *conname, PGresult *res,
! 			 const char *dblink_context_msg, bool fail);
  static char *get_connect_string(const char *servername);
  static char *escape_param_str(const char *from);
  static void validate_pkattnums(Relation rel,
*** dblink_open(PG_FUNCTION_ARGS)
*** 427,433 
  	res = PQexec(conn, buf.data);
  	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
! 		dblink_res_error(conname, res, "could not open cursor", fail);
  		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
  	}
  
--- 428,434 
  	res = PQexec(conn, buf.data);
  	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
! 		dblink_res_error(conn, conname, res, "could not open cursor", fail);
  		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
  	}
  
*** dblink_close(PG_FUNCTION_ARGS)
*** 496,502 
  	res = PQexec(conn, buf.data);
  	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
! 		dblink_res_error(conname, res, "could not close cursor", fail);
  		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
  	}
  
--- 497,503 
  	res = PQexec(conn, buf.data);
  	if (!res || PQresultStatus(res) != PGRES_COMMAND_OK)
  	{
! 		dblink_res_error(conn, conname, res, "could not close cursor", fail);
  		PG_RETURN_TEXT_P(cstring_to_text("ERROR"));
  	}
  
*** dblink_fetch(PG_FUNCTION_ARGS)
*** 599,605 
  		(PQresultStatus(res) != PGRES_COMMAND_OK &&
  		 PQresultStatus(res) != PGRES_TUPLES_OK))
  	{
! 		dblink_res_error(conname, res, "could not fetch from cursor", fail);
  		return (Datum) 0;
  	}
  	else if (PQresultStatus(res) == PGRES_COMMAND_OK)
--- 600,607 
  		(PQresultStatus(res) != PGRES_COMMAND_OK &&
  		 PQresultStatus(res) != PGRES_TUPLES_OK))
  	{
! 		dblink_res_error(conn, conname, res,
! 		 "could not fetch from cursor", fail);
  		return (Datum) 0;
  	}
  	else if (PQresultStatus(res) == PGRES_COMMAND_OK)
*** dblink_record_internal(FunctionCallInfo
*** 750,757 
  if (PQresultStatus(res) != PGRES_COMMAND_OK &&
  	PQresultStatus(res) != PGRES_TUPLES_OK)
  {
! 	dblink_res_error(conname, res, "could not execute query",
! 	 fail);
  	/* if fail isn't set, we'll return an empty query result */
  }
  else
--- 752,759 
  if (PQresultStatus(res) != PGRES_COMMAND_OK &&
  	PQresultStatus(res) != PGRES_TUPLES_OK)
  {
! 	dblin