RE: Libpq support to connect to standby server as priority
Hi Greg,
I have spent some time reading this discussion thread, and doing a code review
of the latest (v17-0001) patch.
Below are my review comments; some are trivial, others not so much.
GENERAL COMMENT 1 ("any")
"any" should be included as valid option for target_server_type.
IIUC target_server_type was added mostly to have better alignment with JDBC
options.
Both Vladimir [1] and Dave [2] already said that JDBC does have an "any" option.
[1] -
https://www.postgresql.org/message-id/CAB%3DJe-FwOVE%3D8gR1UDDZRnWZR65fRG40e8zW_U_6mnUqbce68g%40mail.gmail.com
[2] -
https://www.postgresql.org/message-id/CADK3HHJ9316ji7L-97cJBY%3Dwp4E3ddPMn8XdkNz6j8d9u0OhmQ%40mail.gmail.com
Furthermore, the fe-connect.c function makeEmptyPGConn sets default:
+ conn->requested_server_type = SERVER_TYPE_ANY;
This means the default type of target_server_type is "any".
Since this is default, it should also be possible to assign the same value to
explicitly.
(Parts of the v17 patch affected by this are itemised below)
GENERAL COMMENT 2 (Removal of pg_is_in_recovery)
Around 22/3/2019 Hari added a lot of pg_is_in_recovery code in his patch 0006
[1]
[1] -
https://www.postgresql.org/message-id/CAJrrPGd4YeA%2BN%3DxC%2B1XPVoGzMCATJZY4irVQEJ6i0aPqorUi7g%40mail.gmail.com
Much later IIUC the latest v17 patch has taken onboard the recommendation from
Alvaro and removed all that code:
"I would discard the whole thing about checking "SELECT pg_is_in_recovery()""
[2]
[2] -
https://www.postgresql.org/message-id/20191227130828.GA21647%40alvherre.pgsql
However, it seems that not ALL parts of the original code got cleanly removed
in v17.
There are a number of references to CONNECTION_CHECK_RECOVERY and
pg_is_in_recovery still lurking.
(Parts of the v17 patch affected by this are itemised below)
COMMENT libpq.sgml (para blocks)
+
The v17 patch for target_session_attrs and target_server_type help is currently
using blocks for each of the possible option values.
This format is inconsistent document style with other variables in this SGML.
Other places are using sub-lists for option values. e.g. look at "six modes" of
sslmode.
COMMENT libpq.sgml (cut/paste parameter description)
I don't think that target_server_type help should be just a cut/paste duplicate
of target_session_attrs. It is confusing because it leaves the reader doubting
the purpose of having such a duplication.
Suggest to simplify the target_server_type help like as follows:
--
target_server_type
The purpose of this parameter is to reflect the similar PGJDBC targetServerType.
The supported options are same as target_session_attrs.
This parameter overrides any connection type specified by target_session_attrs.
--
COMMENT libpq.sgml (pg_is_in_recovery)
(As noted in GENERAL COMMENT 2 there are still residual references to
pg_is_in_recovery)
+
+If this parameter is set to standby, only a
connection in which
+the server is in recovery mode is considered acceptable. If the server
is prior to version 14,
+the query SELECT pg_is_in_recovery() will be sent
upon any successful
+connection; if it returns t, it means the server is
in recovery mode.
+
Suggest change to:
--
If this parameter is set to standby, only a connection in
which the server is in recovery mode is considered acceptable. The recovery
mode state is determined by the value of the in_recovery configuration
parameter that is reported by the server upon successful connection. Otherwise,
if the server is prior to version 14, only a connection in which read-write
transactions are not accepted by default is considered acceptable. To determine
whether the server supports read-write transactions, the query SHOW
transaction_read_only will be sent upon any successful connection; if it
returns on, it means the server doesn't support read-write transactions.
--
COMMENT libpq.sgml (Oxford comma)
+ integer_datetimes,
+ standard_conforming_strings and
+ in_recovery.
Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.
COMMENT protocol.sgml (Oxford comma)
+integer_datetimes,
+standard_conforming_strings and
+in_recovery.
Previously there was an Oxford comma (e.g. before the "and"). Now there isn't.
The v17 patch should not alter the previous listing style.
QUESTION standby.c - SendRecoveryExitSignal
+/*
+ * SendRecoveryExitSignal
+ * Signal backends that the server has exited recovery mode.
+ */
+void
+SendRecoveryExitSignal(void)
+{
+ SendSignalToAllBackends(PROCSIG_RECOVERY_EXIT);
+}
I wonder if this function is really necessary?
IIUC the SendRecoveryExitSignal is only called from one place (xlog.c).
Why not just call SendSignalToAllBackends directly from there and remove this
extra layer?
COMMENT postgres.c (signal comment)
+ /* sig
RE: Libpq support to connect to standby server as priority
Hi Greg, I was able to successfully execute all the tests of the v17-0001 patch. But I do have a couple of additional review comments about the test code. COMMENT - missing "any" tests In my earlier code review (previous email) I suggested that "any" should be added as valid option to the target_server_type parameter. But this now means there are some missing test cases for target_server_type = "any" COMMENT - negative tests? IIUC when "standby" (aka "secondary") is specified, and there is no in_recovery server available, then the result should be an error like "could not make a readonly connection to server " But I did not find any such error combination tests. e.g. Where are these test cases? target_session_attrs = "standby", when no standby is available target_session_attrs = "secondary", when no standby is available target_server_type = "standby", when no standby is available target_server_type = "secondary", when no standby is available -- And, similarly for "could not make a writable connection to server ". e.g. Where are these test cases? target_session_attrs = "primary", when no primary is available target_session_attrs = "read-write", when no primary is available target_server_type = "primary", when no primary is available target_server_type = "read-write", when no primary is available Kind Regards, Peter Smith --- Fujitsu Australia
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
-Original Message-
From: Masahiko Sawada Sent: Thursday, 15 August 2019
7:10 PM
> BTW I've created PoC patch for cluster encryption feature. Attached patch set
> has done some items of TODO list and some of them can be used even for finer
> granularity encryption. Anyway, the implemented components are followings:
Hello Sawada-san,
I guess your original patch code may be getting a bit out-dated by the ongoing
TDE discussions, but I have done some code review of it anyway.
Hopefully a few comments below can still be of use going forward:
---
REVIEW COMMENTS
* src/backend/storage/encryption/enc_cipher.c – For functions
EncryptionCipherValue/String maybe should log warnings for unexpected values
instead of silently assigning to default 0/”off”.
* src/backend/storage/encryption/enc_cipher.c – For function
EncryptionCipherString, purpose of returning ”unknown” if unclear because that
will map back to “off” again anyway via EncryptionCipherValue. Why not just
return "off" (with warning logged).
* src/include/storage/enc_common.h – Typo in comment: "Encrypton".
* src/include/storage/encryption.h - The macro DataEncryptionEnabled may be
better to be using enum TDE_ENCRYPTION_OFF instead of magic number 0
* src/backend/storage/encryption/kmgr.c - Function BootStrapKmgr will report
error if USE_OPENSSL is not defined. The check seems premature because it would
fail even if the user is not using encryption. Shouldn't the lack of openssl be
OK when user is not using TDE at all (i.e. when encryption is "none")?
* src/backend/storage/encryption/kmgr.c - In function BootStrapMgr suggest
better to check if (bootstrap_data_encryption_cipher == TDE_ENCRYPTION_OFF)
using enum instead of the magic number 0.
* src/backend/storage/encryption/kmgr.c - The function
run_cluster_passphrase_command function seems mostly a clone of an existing
run_ssl_passphrase_command function. Is it possible to refactor to share the
common code?
* src/backend/storage/encryption/kmgr.c - The function derive_encryption_key
declares a char key_len. Why char? It seems int everywhere else.
* src/backend/bootstrap/bootstrap.c - Suggest better if variable declaration
bootstrap_data_encryption_cipher = 0 uses enum TDE_ENCRYPTION_OFF instead of
magic number 0
* src/backend/utils/misc/guc.c - It looks like the default value for GUC
variable data_encryption_cipher is AES128. Wouldn't "off" be the more
appropriate default value? Otherwise it seems inconsistent with the logic of
initdb (which insists that the -e option is mandatory if you wanted any
encryption).
* src/backend/utils/misc/guc.c - There is a missing entry in the
config_group_names[]. The patch changed the config_group[] in guc_tables.h, so
I think there needs to be a matching item in the config_group_names.
* src/bin/initdb/initdb.c - The function check_encryption_cipher would disallow
an encryption value of "none". Although maybe it is not very useful to say -e
none, it does seem inconsistent to reject it, given that "none" was a valid
value for the GUC variable data_encryption_cipher.
* contrib/bloom/blinsert.c - In function btbuildempty the arguments for
PageEncryptionInPlace seem in the wrong order (forknum should be 2nd).
* src/backend/access/hash/hashpage.c - In function _hash_alloc_buckets the
arguments for PageEncryptionInPlace seem in the wrong order (forknum should be
2nd).
* src/backend/access/spgist/spginsert.c - In function spgbuildempty the
arguments for PageEncryptionInPlace seem in the wrong order (forknum should be
2nd). This error looks repeated 3X.
* in multiple files - The encryption enums have equivalent strings ("off",
"aes-128", "aes-256") but they are scattered as string literals in many places
(e.g. pg_controldata.c, initdb.c, guc.c, enc_cipher.c). Suggest it would be
better to declare those as string constants in one place only.
---
Kind Regards,
Peter Smith
Fujitsu Australia
Proposal: Add more compile-time asserts to expose inconsistencies.
Dear Hackers, I have identified some OSS code where more compile-time asserts could be added. Mostly these are asserting that arrays have the necessary length to accommodate the enums that are used to index into them. In general the code is already commented with warnings such as: * "If you add a new entry, remember to ..." * "When modifying this enum, update the table in ..." * "Display names for enums in ..." * etc. But comments can be accidentally overlooked, so adding the compile-time asserts can help eliminate human error. Please refer to the attached patch. Kind Regards, Peter Smith --- Fujitsu Australia 0001-Add-compile-time-asserts.patch Description: 0001-Add-compile-time-asserts.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message- From: Michael Paquier Sent: Wednesday, 18 September 2019 5:40 PM > For some of them it could help, and we could think about a better location > for that stuff than an unused routine. The indentation of your patch is > weird, with "git diff --check" complaining a lot. > > If you want to discuss more about that, could you add that to the next commit > fest? Here it is: https://commitfest.postgresql.org/25/ Hi Michael, Thanks for your feedback and advice. I have modified the patch to clean up the whitespace issues, and added it to the next commit fest. Kind Regards, --- Peter Smith Fujitsu Australia add_more_ct_asserts.patch Description: add_more_ct_asserts.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message- From: Michael Paquier Sent: Thursday, 19 September 2019 11:08 AM >On Wed, Sep 18, 2019 at 04:46:30PM +0100, Dagfinn Ilmari Mannsåker wrote: >> Postgres doesn't seem to have it, but it would be possible to define a >> StaticAssertDecl macro that can be used at the file level, outside any >> function. See for example Perl's STATIC_ASSERT_DECL: >> >> https://github.com/Perl/perl5/blob/v5.30.0/perl.h#L3455-L3488 > >That sounds like a cleaner alternative. Thanks for the pointer. In the attached patch example I have defined a new macro StaticAssertDecl. A typical usage of it is shown in the relpath.c file. The goal was to leave all existing Postgres static assert macros unchanged, but to allow static asserts to be added in the code at file scope without the need for the explicit ct_asserts function. Notice, in reality the StaticAssertDecl macro still uses a static function as a wrapper for the StaticAssertStmt, but now the function is not visible in the source. If this strategy is acceptable I will update my original patch to remove all those ct_asserts functions, and instead put each StaticAssertDecl nearby the array that it is asserting (e.g. just like relpath.c) Kind Regards, Peter Smith --- Fujitsu Australia add_more_ct_asserts_StaticAssertDecl.patch Description: add_more_ct_asserts_StaticAssertDecl.patch
Proposal: Make use of C99 designated initialisers for nulls/values arrays
Dear Hackers,
I have identified some OSS code which maybe can make use of C99 designated
initialisers for nulls/values arrays.
~
Background:
There are lots of tuple operations where arrays of values and flags are being
passed.
Typically these arrays are being previously initialised 0/false by memset.
By modifying code to use C99 designated initialiser syntax [1], most of these
memsets can become redundant.
Actually, this mechanism is already being used in some of the existing OSS
code. This patch/proposal just propagates the same idea to all other similar
places I could find.
~
Result:
Less code. Removes ~200 unnecessary memsets.
More consistent initialisation.
~
Typical Example:
Before:
Datum values[Natts_pg_attribute];
boolnulls[Natts_pg_attribute];
...
memset(values, 0, sizeof(values));
memset(nulls, false, sizeof(nulls));
After:
Datum values[Natts_pg_attribute] = {0};
boolnulls[Natts_pg_attribute] = {0};
---
[1] REF C99 [$6.7.8/21] If there are fewer initializers in a brace-enclosed
list than there are elements or members of an aggregate,
or fewer characters in a string literal used to initialize an array of known
size than there are elements in the array,
the remainder of the aggregate shall be initialized implicitly the same as
objects that have static storage duration
~
Please refer to the attached patch.
Kind Regards,
---
Peter Smith
Fujitsu Australia
init_nulls.patch
Description: init_nulls.patch
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Isaac Morland Sent: Tuesday, 1 October 2019
11:32 PM
>Typical Example:
>Before:
> Datum values[Natts_pg_attribute];
> bool nulls[Natts_pg_attribute];
> ...
> memset(values, 0, sizeof(values));
> memset(nulls, false, sizeof(nulls));
>After:
> Datum values[Natts_pg_attribute] = {0};
> bool nulls[Natts_pg_attribute] = {0};
>
>I hope you'll forgive a noob question. Why does the "After" initialization for
>the boolean array have {0} rather than {false}?
It is a valid question.
I found that the original memsets that this patch replaces were already using 0
and false interchangeably. So I just picked one.
Reasons I chose {0} over {false} are: (a) laziness, and (b) consistency with
the values[] initialiser.
But it is no problem to change the bool initialisers to {false} if that becomes
a committer review issue.
Kind Regards
--
Peter Smith
Fujitsu Australia
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Amit Kapila Sent: Tuesday, 1 October 2019 8:12 PM > +1. This seems like an improvement. I can review and take this forward > unless there are objections from others. FYI - I created a Commitfest entry for this here: https://commitfest.postgresql.org/25/2290/ Kind Regards -- Peter Smith Fujitsu Australia
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Amit Kapila Sent: Wednesday, 2 October 2019 9:42 AM > I don't have any strong opinion on this, but I would mildly prefer to > initialize boolean array with false just for the sake of readability (we > generally initializing booleans with false). Done. Please see attached updated patch. Kind Regards -- Peter Smith Fujitsu Australia c99_init_nulls_2.patch Description: c99_init_nulls_2.patch
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
-Original Message-
From: Tom Lane Sent: Thursday, 3 October 2019 1:46 AM
> Right. I think that in general it's bad practice for an initializer to not
> specify all fields/elements of the target.
> It is okay in the specific case that we're substituting for a memset(..., 0,
> ...).
> Perhaps we could make this explicit by using a coding style like
>
>/* in c.h or some such place: */
>#define INIT_ALL_ZEROES {0}
>
>/* in code: */
> Datum values[N] = INIT_ALL_ZEROES;
The patch has been updated per your suggestion. Now using macros for these
partial initialisers.
Please see attachment.
Kind Regards
---
Peter Smith
Fujitsu Australia
c99_init_nulls_3.patch
Description: c99_init_nulls_3.patch
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Amit Kapila Sent: Friday, 4 October 2019 1:32 PM
> Also, if we want to pursue, do we want to use INIT_ALL_ZEROES for bool
>arrays as well?
FYI - In case it went unnoticed - my last patch addresses this by defining 2
macros:
#define INIT_ALL_ELEMS_ZERO {0}
#define INIT_ALL_ELEMS_FALSE{false}
Kind Regards
--
Peter Smith
Fujitsu Australia
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Tom Lane Sent: Friday, 4 October 2019 2:08 PM
>> #define INIT_ALL_ELEMS_ZERO {0}
>> #define INIT_ALL_ELEMS_FALSE {false}
>I would say that's 100% wrong. The entire point here is that it's
>memset-equivalent, and therefore writes zeroes, regardless of what the
>datatype is.
I agree it is memset-equivalent.
All examples of the memset code that INIT_ALL_ELEMS_ZERO replaces looked like
this:
memset(values, 0, sizeof(values));
Most examples of the memset code that INIT_ALL_ELEMS_FALSE replaces looked like
this:
memset(nulls, false, sizeof(nulls));
~
I made the 2nd macro because I anticipate the same folk that don't like setting
0 to a bool will also not like setting something called INIT_ALL_ELEMS_ZERO to
a bool array.
How about I just define them both the same?
#define INIT_ALL_ELEMS_ZERO {0}
#define INIT_ALL_ELEMS_FALSE{0}
>As a counterexample, the coding you have above looks a lot like it would work
>to add
>
>#define INIT_ALL_ELEMS_TRUE{true}
> which as previously noted will *not* work. So I think the one-size-fits-all
> approach is what to use.
I agree it looks that way; in my previous email I should have provided more
context to the code.
Below is the full fragment of the last shared patch, which included a note to
prevent anybody from doing such a thing.
~~
/*
* Macros for C99 designated-initialiser syntax to set all array elements to
0/false.
*
* Use these macros in preference to explicit {0} syntax to avoid giving a
misleading
* impression that the same value is always used for all elements.
* e.g.
* bool foo[2] = {false}; // sets both elements false
* bool foo[2] = {true}; // does NOT set both elements true
*
* Reference: C99 [$6.7.8/21] If there are fewer initializers in a
brace-enclosed list than there
* are elements or members of an aggregate, or fewer characters in a string
literal used to
* initialize an array of known size than there are elements in the array, the
remainder of the
* aggregate shall be initialized implicitly the same as objects that have
static storage duration
*/
#define INIT_ALL_ELEMS_ZERO {0}
#define INIT_ALL_ELEMS_FALSE{false}
~~
Kind Regards,
--
Peter Smith
Fujitsu Australia
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
From: Amit Kapila Sent: Friday, 4 October 2019 4:50 PM
>>How about I just define them both the same?
>>#define INIT_ALL_ELEMS_ZERO {0}
>>#define INIT_ALL_ELEMS_FALSE {0}
>
>I think using one define would be preferred, but you can wait and see if
>others prefer defining different macros for the same thing.
While nowhere near unanimous, it seems majority favour using a macro (if only
to protect the unwary and document the behaviour).
And of those in favour of macros, using INIT_ALL_ELEMS_ZERO even for bool array
is a clear preference.
So, please find attached the updated patch, which now has just 1 macro.
Kind Regards
--
Peter Smith
Fujitsu Australia
c99_init_nulls_4.patch
Description: c99_init_nulls_4.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Tuesday, 1 October 2019 3:14 AM >I wonder if defining the fallback static assert code to something like > extern void static_assert_func(int static_assert_failed[(condition) ? 1 : > -1]); isn't a solution, however. I *think* that's standard C. Seems to work > at least with gcc, clang, msvc, icc. > >Re standard: C99's "6.7 Declarations" + 6.7.1 defines 'declaration' to include >extern specifiers and in 6.7.1 5) says "The declaration of an identifier for a >function that has block scope shall have >no explicit storage-class specifier >other than extern.". And "6.8 Statements and blocks", via "6.8.2 Compound >statement" allows declarations in statements. > >You can play with a good few compilers at: https://godbolt.org/z/fl0Mzu I liked your idea of using an extern function declaration for implementing the file-scope compile-time asserts. AFAIK it is valid standard C. Thank you for the useful link to that compiler explorer. I tried many scenarios of the new StaticAssertDecl and all seemed to work ok. https://godbolt.org/z/fDrmXi The patch has been updated accordingly. All assertions identified in the original post are now adjacent the global variables they are asserting. Kind Regards -- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_2.patch Description: ct_asserts_StaticAssertDecl_2.patch
RE: New SQL counter statistics view (pg_stat_sql)
Dear Hackers,
We have resurrected this 2 year old "SQL statements statistics counter"
proposal from Hari.
The attached patch addresses the previous review issues.
The commit-fest entry has been moved forward to the next commit-fest
https://commitfest.postgresql.org/25/790/
Please review again, and consider if this is OK for "Ready for Committer"
status.
Kind Regards
--
Peter Smith
Fujitsu Australia
-Original Message-
From: pgsql-hackers-ow...@postgresql.org
On Behalf Of Andres Freund
Sent: Thursday, 6 April 2017 5:18 AM
To: Haribabu Kommi
Cc: Michael Paquier ; Dilip Kumar
; vinayak ;
pgsql-hack...@postgresql.org
Subject: Re: New SQL counter statistics view (pg_stat_sql)
Hi,
I'm somewhat inclined to think that this really would be better done in an
extension like pg_stat_statements.
On 2017-03-08 14:39:23 +1100, Haribabu Kommi wrote:
> On Wed, Feb 1, 2017 at 3:13 PM, Michael Paquier
>
> +
> + track_sql (boolean)
> +
> + track_sql configuration parameter
> +
> +
> +
> +
> +Enables collection of different SQL statement statistics that are
> +executed on the instance. This parameter is off by default. Only
> +superusers can change this setting.
> +
> +
> +
> +
I don't like this name much, seems a bit too generic to me. 'track_activities',
'track_io_timings' are at least a bit clearer.
How about track_statement_statistics + corresponding view/function renaming?
> +
> + pg_stat_sql View
> +
> +
> +
> + Column
> + Type
> + Description
> +
> +
> +
> +
> +
> + tag
> + text
> + Name of the SQL statement
> +
It's not really the "name" of a statement. Internally and IIRC elsewhere in the
docs we describe something like this as tag?
> +/* --
> + * pgstat_send_sqlstmt(void)
> + *
> + * Send SQL statement statistics to the collector
> + * --
> + */
> +static void
> +pgstat_send_sqlstmt(void)
> +{
> + PgStat_MsgSqlstmt msg;
> + PgStat_SqlstmtEntry *entry;
> + HASH_SEQ_STATUS hstat;
> +
> + if (pgstat_backend_sql == NULL)
> + return;
> +
> + pgstat_setheader(&msg.m_hdr, PGSTAT_MTYPE_SQLSTMT);
> + msg.m_nentries = 0;
> +
> + hash_seq_init(&hstat, pgstat_backend_sql);
> + while ((entry = (PgStat_SqlstmtEntry *) hash_seq_search(&hstat)) !=
> NULL)
> + {
> + PgStat_SqlstmtEntry *m_ent;
> +
> + /* Skip it if no counts accumulated since last time */
> + if (entry->count == 0)
> + continue;
> +
> + /* need to convert format of time accumulators */
> + m_ent = &msg.m_entry[msg.m_nentries];
> + memcpy(m_ent, entry, sizeof(PgStat_SqlstmtEntry));
> +
> + if (++msg.m_nentries >= PGSTAT_NUM_SQLSTMTS)
> + {
> + pgstat_send(&msg, offsetof(PgStat_MsgSqlstmt,
> m_entry[0]) +
> + msg.m_nentries *
> sizeof(PgStat_SqlstmtEntry));
> + msg.m_nentries = 0;
> + }
> +
> + /* reset the entry's count */
> + entry->count = 0;
> + }
> +
> + if (msg.m_nentries > 0)
> + pgstat_send(&msg, offsetof(PgStat_MsgSqlstmt, m_entry[0]) +
> + msg.m_nentries *
> sizeof(PgStat_SqlstmtEntry));
Hm. So pgstat_backend_sql is never deleted, which'll mean that if a backend
lives long we'll continually iterate over a lot of 0 entries. I think
performance evaluation of this feature should take that into account.
> +}
> +
> +/*
> + * Count SQL statement for pg_stat_sql view */ void
> +pgstat_count_sqlstmt(const char *commandTag) {
> + PgStat_SqlstmtEntry *htabent;
> + boolfound;
> +
> + if (!pgstat_backend_sql)
> + {
> + /* First time through - initialize SQL statement stat table */
> + HASHCTL hash_ctl;
> +
> + memset(&hash_ctl, 0, sizeof(hash_ctl));
> + hash_ctl.keysize = NAMEDATALEN;
> + hash_ctl.entrysize = sizeof(PgStat_SqlstmtEntry);
> + pgstat_backend_sql = hash_create("SQL statement stat entries",
> +
> PGSTAT_SQLSTMT_HASH_SIZE,
> +
> &hash_ctl,
> +
> HASH_ELEM | HASH_BLOBS);
> + }
> +
> + /* Get the stats entry for this SQL statement, create if necessary */
> + htabent = hash_search(pgstat_backend_sql, commandTag,
> + HASH_ENTER, &found);
> + if (!found)
> + htabent->count = 1;
> + else
> + htabent->count++;
> +}
That's not really something in this patch, but a lot of this would be
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Hi Masahiko,
> Let me briefly explain the current design I'm thinking. The design employees
> 2-tier key architecture. That is, a database cluster has one
> master key and per-tablespace keys which are encrypted with the master key
> before storing to disk. Each tablespace keys are generated
> randomly inside database when CREATE TABLESPACE. The all encrypted tablespace
> keys are stored together with the master key ID to the
> file (say, $PGDATA/base/pg_tblsp_keys). That way, the startup process can
> easily get all tablespace keys and the master key ID before
> starting recovery, and therefore can get the all decrypted tablespace keys.
Your design idea sounds very similar to the current Fujitsu Enterprise Postgres
(FEP) implementation of TDE.
FEP uses a master encryption key (MEK) for the database cluster. This MEK is
stored in a file at some GUC variable location. This file is encrypted using a
“passphrase” known only to the administrator.
There are also per-tablespace keys, which are randomly generated at the time of
CREATE TABLESPACE and stored in files. There is one tablespace key file per
tablespace. These tablespace key files are encrypted by the MEK and stored at
the location specified by CREATE TABLESPACE.
Not all tablespaces use TDE. An FEP extension of the CREATE TABLESPACE syntax,
creates the tablespace key file only when encryption was requested.
e.g. CREATE TABLESPACE my_secure_tablespace LOCATION
'/home/postgre/FEP/TESTING/tablespacedir' WITH (tablespace_encryption_algorithm
= 'AES256');
The MEK is not currently got from a third party. It is randomly generated when
the master key file is first created by another added function.
e.g. select pgx_set_master_key('passphrase');
Kind Regards,
Peter Smith
Fujitsu Australia
Disclaimer
The information in this e-mail is confidential and may contain content that is
subject to copyright and/or is commercial-in-confidence and is intended only
for the use of the above named addressee. If you are not the intended
recipient, you are hereby notified that dissemination, copying or use of the
information is strictly prohibited. If you have received this e-mail in error,
please telephone Fujitsu Australia Software Technology Pty Ltd on + 61 2 9452
9000 or by reply e-mail to the sender and delete the document and all copies
thereof.
Whereas Fujitsu Australia Software Technology Pty Ltd would not knowingly
transmit a virus within an email communication, it is the receiver’s
responsibility to scan all communication and any files attached for computer
viruses and other defects. Fujitsu Australia Software Technology Pty Ltd does
not accept liability for any loss or damage (whether direct, indirect,
consequential or economic) however caused, and whether by negligence or
otherwise, which may result directly or indirectly from this communication or
any files attached.
If you do not wish to receive commercial and/or marketing email messages from
Fujitsu Australia Software Technology Pty Ltd, please email
unsubscr...@fast.au.fujitsu.com
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
On Sat, Aug 10, 2019 at 1:19 AM Tomas Vondra wrote: > We can of course support "forced" re-encryption, but I think it's acceptable > if that's fairly expensive as long as it can be throttled and executed in the > background (kinda similar to the patch to enable checksums in the background). As an alternative way to provide for a "forced" re-encryption couldn't you just run pg_dumpall + psql? Regards, -- Peter Smith Fujitsu Australia
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
> From: Ibrar Ahmed Sent: Sunday, 18 August 2019 2:43 AM > +1 for voice call, bruce we usually have a weekly TDE call. I will include > you in If you don't mind, please also add me to that TDE call list. Thanks/Regards, --- Peter Smith Fujitsu Australia
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
> -Original Message- > From: Stephen Frost Sent: Friday, 16 August 2019 11:01 AM > Having direct integration with a KMS would certainly be valuable, and I don't > see a reason to deny users that option if someone would like to spend time > implementing it- in addition to a simpler mechanism such as a passphrase > command, which I believe is what was being suggested here. Yes. We recently made an internal PoC for FEP to enable it to reach out to AWS KMS whenever the MKEY was rotated or TDKEY was created. This was achieved by inserting some hooks in our TDE code - these hooks were implemented by a contrib-module loaded by the shared_preload_libraries GUC variable. So when no special "tdekey_aws" module was loaded, our TDE functionality simply reverts to its default (random) MDEK/TDEK keys. Even if OSS community chooses not to implement any KMS integration, the TDE design could consider providing hooks in a few appropriate places to make it easy for people who may need to add their own later. Regards, --- Peter Smith Fujitsu Australia
RE: [Proposal] Table-level Transparent Data Encryption (TDE) and Key Management Service (KMS)
Greetings, (Apologies for any naïve thoughts below. Please correct my misunderstandings) I am trying to understand the background for the ideas proposed and/or already decided, but it is increasingly difficult to follow. I’ve been watching the TDE list for several months and over that time there have been dozens of different ideas floated; Each of them have their own good points; Some are conflicting; IMO any TDE implementation will be a trade-off between a number of factors: * Design – e.g. Simple v Complex solution * Secureness – e.g. Acceptance that a simpler solution may not handle every possible threat * Cost/Feasibility – e.g. How hard will TDE be to implement/maintain. * User expectations - e.g. What is the “threat model” the end user actually wants to protect against * User expectations – e.g. Comparison with other products * Completeness – e.g. Acknowledgement that first implementation may not meet the end-goal. * Future proof – e.g. ability to evolve in future TDE versions (with minimal re-write of what came before) * Usability – e.g. Online/offline considerations * Usability – e.g. Will a more complex solution end up being too difficult to actually use/administer * etc… New TDE ideas keep popping up all the time. The discussion sometimes has become mired in technical details; I’m losing sight of the bigger picture. Would it be possible to share a *tabulation* for all the TDE components; Each component may be a number of design choices (options); And have brief lists of Pros/Cons for each of those options so that each can be concisely summarised on their respective merits. I think this would be of a great help to understand how we got to where we are now, as well as helping to focus on how to proceed. For example, = Component: TDKEY * Option: use derived keys; List of Pros/Cons * Option: use random keys; List of Pros/Cons * Option: use keys from some external source and encrypted by MDKEY; List of Pros/Cons * Option: use same TKEY for all tables/tablespaces; List of Pros/Cons * Option: … * Option: … * => Decision (i.e. the least-worst compromise/combination of the possible options) = ~ Postscript: After writing this, I recalled recently reading a mail from Antonin https://www.postgresql.org/message-id/44057.1565977657%40antos which says pretty much the same thing! Also, I recognise that there is an offline shared Googledoc which already includes some of this information, but I think it would be valuable if it could be formatted as Pros/Cons summary table and shared on the Wiki page for everybody to see. Kind Regards, --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi Michael, Sorry I was AWOL for a couple of months. Thanks for taking the patch further, and committing it. Kind Regards --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Sunday, 27 October 2019 12:03 PM >>Ideally, the implementation should end up calling _Static_assert() >>somehow, so that we get the compiler's native error message. OK. I can work on that. >>We could do a configure check for whether _Static_assert() works at >>file scope. I don't know what the support for that is, but it seems to >>work in gcc and clang > I think it should work everywhere that has static assert. So we should need a > separate configure check. Er, that's a typo right? I think you meant: "So we *shouldn't* need a separate configure check" Kind Regards --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Sunday, 27 October 2019 12:03 PM > My proposal for this really was just to use this as a fallback for when > static assert isn't available. Which in turn I was just suggesting because > Tom wanted a fallback. The patch is updated to use "extern" technique only when "_Static_assert" is unavailable. PSA. Kind Regards, --- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_3.patch Description: ct_asserts_StaticAssertDecl_3.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Kyotaro Horiguchi Sent: Monday, 28 October 2019 1:26 PM > It is missing the __cplusplus case? My use cases for the macro are only in C code, so that's all I was interested in at this time. If somebody else wants to extend the patch for C++ also (and test it) they can do. Kind Regards, --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
From: Andres Freund Sent: Wednesday, 13 November 2019 6:01 AM >Peter Smith: > > Is there a reason to not just make StaticAssertExpr and StaticAssertStmt be > the same? I don't want to proliferate variants that users have to understand > if there's no compelling > need. Nor do I think do we really need two different fallback implementation > for static asserts. > > As far as I can tell we should be able to use the prototype based approach in > all the cases where we currently use the "negative bit-field width" approach? I also thought that the new "prototype negative array-dimension" based approach (i.e. StaticAssertDecl) looked like an improvement over the existing "negative bit-field width" approach (i.e. StaticAssertStmt), because it seems to work for more scenarios (e.g. file scope). But I did not refactor existing code to use the new way because I was fearful that there might be some subtle reason why the StaticAssertStmt was deliberately made that way (e.g. as do/while), and last thing I want to do was break working code. > Should then also update > * Otherwise we fall back on a kluge that assumes the compiler will complain > * about a negative width for a struct bit-field. This will not include a > * helpful error message, but it beats not getting an error at all. Kind Regards. Peter Smith --- Fujitsu Australia
RE: New SQL counter statistics view (pg_stat_sql)
From: Thomas Munro Sent: Monday, 4 November 2019 1:43 PM > No comment on the patch but I noticed that the documentation changes don't > build. Please make sure you can "make docs" successfully, having installed > the documentation tools[1]. Thanks for the feedback. An updated patch which fixes the docs issue is attached. Kind Regards. Peter Smith --- Fujitsu Australia 0001-pg_stat_sql.patch Description: 0001-pg_stat_sql.patch
RE: Proposal: Make use of C99 designated initialisers for nulls/values arrays
Hi Hackers.
This submission seems to have stalled.
Please forgive this post - I am unsure if the submission process expects me to
come to defence of my own patch for one last gasp, or if I am supposed to just
sit back and watch it die a slow death of a thousand cuts.
I thought this submission actually started out very popular, but then support
slowly eroded, and currently seems headed towards a likely rejection.
~
Anyway, here are my arguments:
(a) I recognise that on first glance, the {0} syntax might evoke a momentary
"double-take" by the someone reading the code. IMO this would only be
experienced by somebody encountering {0} syntax for the very first time. This
is not an really uncommon "pattern" (it's already elsewhere in PostreSQL code),
and once you've seen it two or three times there is no confusion what it is
doing.
(b) Because of (a) I don't really agree with the notion that it should be
replaced by a macro to hide the C syntax. I did try adding various macros as
suggested, but all that achieved was to was spin off another 20 emails debating
the macro format. I thought any code committer/reviewer should have no trouble
at all to understand standard C syntax.
(c) It was never a goal of this submission that *all* memsets should be
replaced by {0}. Sometimes {0} is more concise and better IMO, but sometimes
memset is a way more appropriate choice. This patch only replaces simple
examples of primitive types like the values[] and nulls[] arrays (which was a
repeated pattern for many tuple operations). I think any concern that {0} may
not work for all other complex cases is a red-herring. When memset is better,
then use memset.
(d) Wishing for C99 syntax to be same as the simpler {} style of C++ is another
red-herring. I can only use what is officially supported. It is what it is.
(e) The PostgreSQL miscellaneous coding conventions -
https://www.postgresql.org/docs/current/source-conventions.html - says to avoid
" intermingled declarations and code". This leads to some code where the
variable declaration and the initialization (e.g. memset 0 or memset false)
code can be widely separated. It can be an easy source of mistakes to assume a
variable was already initialized when maybe it wasn't. This patch puts the
initialization at the point of declaration, and so eliminates this risk. Isn't
that best practice?
(f) I'm still a bit perplexed how can it be that removing 200 lines of
unnecessary function calls is not considered a good thing to do? Are patches
that only tidy up code generally not accepted? I don't know.
~
That's all I have to say in support of my patch; it will live or it will die
according to the community wish.
If nothing else, at least I've learned a new term - "bike shedding" :-)
Kind Regards.
---
Peter Smith
Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hi Andres, >>> As far as I can tell we should be able to use the prototype based approach >>> in all the cases where we currently use the "negative bit-field width" >>> approach? >> ... >> But I did not refactor existing code to use the new way because I was >> fearful that there might be some subtle reason why the >> StaticAssertStmt was deliberately made that way (e.g. as do/while), >> and last thing I want to do was break working code. >That'll just leave us with cruft. And it's not like this stuff will break in a >subtle way or such FYI - I did try, per your suggestion, to replace the existing StaticAssertStmt to also use the same fallback "extern" syntax form as the new StaticAssertDecl, but the code broke as I suspected it might do: path.c: In function 'first_dir_separator': ../../src/include/c.h:847:2: error: expected expression before 'extern' extern void static_assert_func(int static_assert_failure[(condition) ? 1 : -1]) ^ ../../src/include/c.h:849:2: note: in expansion of macro 'StaticAssertStmt' StaticAssertStmt(condition, errmessage) ^ ../../src/include/c.h:1184:3: note: in expansion of macro 'StaticAssertExpr' (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const underlying_type), \ ^ path.c:127:11: note: in expansion of macro 'unconstify' return unconstify(char *, p); ^ Kind Regards. --- Peter Smith Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
> It seems to me that there is a good point to be consistent with the treatment > of StaticAssertStmt and StaticAssertExpr in c.h, which have fallback > implementations in *all* the configurations supported. Consistency is good, but: * That is beyond the scope for what I wanted my patch to achieve; my use-cases are C code only * It is too risky for me to simply cut/paste my C version of StaticAssertDecl and hope it will work OK for C++. It needs lots of testing because there seems evidence that bad things can happen. E.g. Peter Eisentraut wrote "if you're asking, why is the fallback implementation in C++ different from the one in C, then that's because the C variant didn't work in C++." ~ I am happy if somebody else with more ability to test C++ properly wants to add the __cplusplus variant of the new macro. Meanwhile, I've attached latest re-based version of this patch. Kind Regards. -- Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_4.patch Description: ct_asserts_StaticAssertDecl_4.patch
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
-Original Message-
From: Andres Freund Sent: Tuesday, 3 December 2019 2:56 AM
> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1),
> + "LockTagTypeNames array inconsistency");
> +
>
>These error messages strike me as somewhat unhelpful. I'd probably just reword
>them as "array length mismatch" or something like that.
OK. I have no problem to modify all my current assertion messages to your
suggested text ("array length mismatch") if you think it is better.
Please correct me if I am wrong, but I didn't think the error message text is
of very great significance here because it is a compile-time issue meaning the
*only* person who would see the message is the 1 developer who accidentally
introduced a bug just moments beforehand. The compile will fail with a source
line number, and when the developer sees the StaticAssertDecl at that source
line the cause of the error is anyway self-evident by the condition parameter.
Kind Regards
--
Peter Smith
Fujitsu Australia
RE: Proposal: Add more compile-time asserts to expose inconsistencies.
Hello Michael, > In short, attached is an updated version of your patch which attempts to > solve that. I have tested this with some cplusplus stuff, and GCC for both > versions (static_assert is available in GCC >= 6, but a manual change of c.h > does the trick). > I have edited the patch a bit while on it, your assertions did not use > project-style grammar, the use of parenthesis was inconsistent (see relpath.c > for example), and pgindent has complained a bit. Thanks for your updates. ~~ Hello Andres, >> +StaticAssertDecl(lengthof(LockTagTypeNames) == (LOCKTAG_ADVISORY + 1), >> + "LockTagTypeNames array inconsistency"); >> + > These error messages strike me as somewhat unhelpful. I'd probably just > reword them as "array length mismatch" or something like that. I updated the most recent patch (_5 from Michael) so it now has your suggested error message rewording. PSA patch _6 Kind Regards Peter Smith Fujitsu Australia ct_asserts_StaticAssertDecl_6.patch Description: ct_asserts_StaticAssertDecl_6.patch
