Re: psql - add SHOW_ALL_RESULTS option
Hello Peter, With this "voluntary crash", the regression test output is now psql ... ok (test process exited with exit code 2) 281 ms Normally, I'd expect this during development if there was a crash somewhere, but showing this during a normal run now, and moreover still saying "ok", Well, from a testing perspective, the crash is voluntary and it is indeed ok:-) is quite weird and confusing. Maybe this type of test should be done in the TAP framework instead. It could. Another simpler option: add a "psql_voluntary_crash.sql" with just that test instead of modifying the "psql.sql" test script? That would keep the test exit code information, but the name of the script would make things clearer? Also, if non zero status do not look so ok, should they be noted as bad? -- Fabien.
Re: Per-table storage parameters for TableAM/IndexAM extensions
On Wed, Dec 29, 2021 at 10:38 PM Sadhuprasad Patro
wrote:
> Hi,
>
> Currently all the storage options for a table are very much specific
> to the heap but a different AM might need some user defined AM
> specific parameters to help tune the AM. So here is a patch which
> provides an AM level routine so that instead of getting parameters
> validated using “heap_reloptions” it will call the registered AM
> routine.
>
>
This is a good idea. +1.
e.g:
> -- create a new access method and table using this access method
> CREATE ACCESS METHOD myam TYPE TABLE HANDLER ;
>
> CREATE TABLE mytest (a int) USING myam ;
>
> --a new parameter is to set storage parameter for only myam as below
> ALTER TABLE mytest(squargle_size = '100MB');
>
I syntax here is, ALTER TABLE SET ( attribute_option = value
);
> The user-defined parameters will have meaning only for the "myam",
> otherwise error will be thrown. Our relcache already allows the
> AM-specific cache to be stored for each relation.
>
> Open Question: When a user changes AM, then what should be the
> behavior for not supported storage options? Should we drop the options
> and go with only system storage options?
> Or throw an error, in which case the user has to clean the added
> parameters.
>
I think throwing an error makes more sense, so that the user can clean
that.
Here are a few quick cosmetic review comments:
1)
> @@ -1372,7 +1373,8 @@ untransformRelOptions(Datum options)
> */
> bytea *
> extractRelOptions(HeapTuple tuple, TupleDesc tupdesc,
> - amoptions_function amoptions)
> + amoptions_function amoptions,
> + reloptions_function taboptions)
>
Indentation has been changed and needs to be fixed.
2)
case RELKIND_MATVIEW:
> options = table_reloptions(taboptions, classForm->relkind,
> datum, false);
> break;
>
Going beyond line limit.
3)
diff --git a/src/backend/access/heap/heapam_handler.c
> b/src/backend/access/heap/heapam_handler.c
> index 9befe01..6324d7e 100644
> --- a/src/backend/access/heap/heapam_handler.c
> +++ b/src/backend/access/heap/heapam_handler.c
> @@ -2581,6 +2581,7 @@ static const TableAmRoutine heapam_methods = {
> .index_build_range_scan = heapam_index_build_range_scan,
> .index_validate_scan = heapam_index_validate_scan,
>
> + .taboptions = heap_reloptions,
>
Instead of taboptions can name this as relation_options to be in sink with
other members.
4)
@@ -2427,7 +2428,7 @@ do_autovacuum(void)
> */
> MemoryContextSwitchTo(AutovacMemCxt);
> tab = table_recheck_autovac(relid, table_toast_map, pg_class_desc,
> - effective_multixact_freeze_max_age);
> + classRel->rd_tableam->taboptions, effective_multixact_freeze_max_age);
> if (tab == NULL)
>
Split the another added parameter to function in the next line.
5)
Overall patch has many indentation issues, I would suggest running the
pgindent to fix those.
Regards
Rushabh Lathia
www.EnterpriseDB.com
Re: row filtering for logical replication
On Fri, Dec 31, 2021 at 12:39 AM houzj.f...@fujitsu.com wrote: > > On Wed, Dec 29, 2021 11:16 AM Tang, Haiying wrote: > > On Mon, Dec 27, 2021 9:16 PM houzj.f...@fujitsu.com > > wrote: > > > > > > On Thur, Dec 23, 2021 4:28 PM Peter Smith wrote: > > > > Here is the v54* patch set: > > > > > > Attach the v55 patch set which add the following testcases in 0003 patch. > > > 1. Added a test to cover the case where TOASTed values are not included > > > in the > > >new tuple. Suggested by Euler[1]. > > > > > >Note: this test is temporarily commented because it would fail without > > >applying another bug fix patch in another thread[2] which log the > > detoasted > > >value in old value. I have verified locally that the test pass after > > >applying the bug fix patch[2]. > > > > > > 2. Add a test to cover the case that transform the UPDATE into INSERT. > > Provided > > >by Tang. > > > > > > > Thanks for updating the patches. > > > > A few comments: ... > > 3) v55-0002 > > +static bool pgoutput_row_filter_update_check(enum > > ReorderBufferChangeType changetype, Relation relation, > > + > >HeapTuple oldtuple, HeapTuple newtuple, > > + > >RelationSyncEntry *entry, ReorderBufferChangeType *action); > > > > Do we need parameter changetype here? I think it could only be > > REORDER_BUFFER_CHANGE_UPDATE. > > I didn't change this, I think it might be better to wait for Ajin's opinion. I agree with Tang. AFAIK there is no problem removing that redundant param as suggested. BTW - the Assert within that function is also incorrect because the only possible value is REORDER_BUFFER_CHANGE_UPDATE. I will make these fixes in a future version. -- Kind Regards, Peter Smith. Fujitsu Australia.
Re: psql: \dl+ to list large objects privileges
On Tue, Aug 31, 2021 at 05:14:12PM +0300, Pavel Luzanov wrote: > I decided to move the do_lo_list function to describe.c in order to use the > printACLColumn helper function. And at the same time I renamed do_lo_list to > listLargeObjects to unify with the names of other similar functions. The tabs were changed to spaces when you moved the function. I suggest to move the function in a separate 0001 commit, which makes no code changes other than moving from one file to another. A committer would probably push them as a single patch, but this makes it easier to read and review the changes in 0002. Possibly like git diff HEAD~:src/bin/psql/large_obj.c src/bin/psql/describe.c > +if (pset.sversion >= 9) Since a few weeks ago, psql no longer supports server versions before 9.2, so the "if" branch can go away. > I don't like how I handled the + modifier in the \lo_list command. But I > don't know how to do better now. This is the second time I've programmed in > C. The first time was the 'Hello World' program. So maybe something is done > wrong. I think everywhere else just uses verbose = strchr(cmd, '+') != 0; -- Justin
Re: Inconsistent ellipsis in regression test error message?
On Tue, Dec 28, 2021 at 4:34 AM Tom Lane wrote:
>
> Peter Smith writes:
> > The most recent cfbot run for a patch I am interested in has failed a
> > newly added regression test.
> > Please see http://cfbot.cputube.org/ for 36/2906
> > The failure logs [2] are very curious because the error message is
> > what was expected but it has a different position of the ellipsis
>
> That "expected" output is clearly completely insane; it's pointing
> the cursor in the middle of the "TABLE" keyword, not at the offending
> constant. I can reproduce that when the database encoding is UTF8,
> but if it's SQL_ASCII or a single-byte encoding then I get a saner result:
>
> regression=# ALTER PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE
> (1234);
> ERROR: argument of PUBLICATION WHERE must be type boolean, not type integer
> LINE 1: ...PUBLICATION testpub5 SET TABLE testpub_rf_tbl3 WHERE (1234);
> ^
>
> This is not a client-side problem: the error position being reported
> by the server is different, as you can easily see in the server's log:
>
> 2021-12-27 12:05:15.395 EST [1510837] ERROR: argument of PUBLICATION WHERE
> must be type boolean, not type integer at character 33
> 2021-12-27 12:05:15.395 EST [1510837] STATEMENT: ALTER PUBLICATION testpub5
> SET TABLE testpub_rf_tbl3 WHERE (1234);
>
> (it says "at character 61" in the sane case).
>
> I traced this as far as finding that the pstate being passed to
> coerce_to_boolean has a totally wrong p_sourcetext:
>
> (gdb) p *pstate
> $3 = {parentParseState = 0x0,
> p_sourcetext = 0x1fba9e8 "{A_CONST :val 1234 :location 60}",
> p_rtable = 0x2063ce0, p_joinexprs = 0x0, p_joinlist = 0x0,
> p_namespace = 0x2063dc8, p_lateral_active = false, p_ctenamespace = 0x0,
> p_future_ctes = 0x0, p_parent_cte = 0x0, p_target_relation = 0x0,
> p_target_nsitem = 0x0, p_is_insert = false, p_windowdefs = 0x0,
> p_expr_kind = EXPR_KIND_NONE, p_next_resno = 1, p_multiassign_exprs = 0x0,
> p_locking_clause = 0x0, p_locked_from_parent = false,
> p_resolve_unknowns = true, p_queryEnv = 0x0, p_hasAggs = false,
> p_hasWindowFuncs = false, p_hasTargetSRFs = false, p_hasSubLinks = false,
> p_hasModifyingCTE = false, p_last_srf = 0x0, p_pre_columnref_hook = 0x0,
> p_post_columnref_hook = 0x0, p_paramref_hook = 0x0,
> p_coerce_param_hook = 0x0, p_ref_hook_state = 0x0}
>
> In short, GetTransformedWhereClause is inserting completely faulty data in
> p_sourcetext. This code needs to be revised to pass down the original
> command string, or maybe better pass down the whole ParseState that was
> available to AlterPublication, instead of inventing a bogus one.
>
> The reason why the behavior depends on DB encoding is left as an
> exercise for the student.
>
Thanks for the information, and sorry for taking up your time tracing
what ended up being our bug after all...
--
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: Allow escape in application_name
At Tue, 4 Jan 2022 12:36:32 +0900, Masahiko Sawada wrote in > On Tue, Jan 4, 2022 at 12:05 PM Kyotaro Horiguchi > wrote: > > pg_terminate_backend returns just after kill() returns. So I'm afraid > > that there's a case where the next access to ft6 happens before the > > old connection actually ends on slow machines or under heavy load. > > The test does pg_terminate_backend() with a timeout, and in this case, > don't we wait for the backend to end after sending the signal? Oops! I missed that part. I agree that it works. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: row filtering for logical replication
On Mon, Dec 27, 2021 at 9:57 AM Euler Taveira wrote: > > On Sun, Dec 26, 2021, at 1:09 PM, Alvaro Herrera wrote: > > On 2021-Dec-26, Euler Taveira wrote: > > > On Sat, Dec 25, 2021, at 1:20 AM, Amit Kapila wrote: > > > On Fri, Dec 24, 2021 at 11:04 AM Peter Smith > > > wrote: > > > > > > > > So, IMO the PG docs wording for this part should be relaxed a bit. > > > > > > > > e.g. > > > > BEFORE: > > > > + A nullable column in the WHERE clause could > > > > cause the > > > > + expression to evaluate to false; avoid using columns without > > > > not-null > > > > + constraints in the WHERE clause. > > > > AFTER: > > > > + A nullable column in the WHERE clause could > > > > cause the > > > > + expression to evaluate to false. To avoid unexpected results, any > > > > possible > > > > + null values should be accounted for. > > Is this actually correct? I think a null value would cause the > expression to evaluate to null, not false; the issue is that the filter > considers a null value as not matching (right?). Maybe it's better to > spell that out explicitly; both these wordings seem distracting. > > [Reading it again...] I think it is referring to the > pgoutput_row_filter_exec_expr() return. That's not accurate because it is > talking about the expression and the expression returns true, false and null. > However, the referred function returns only true or false. I agree that we > should explictily mention that a null return means the row won't be published. > > You have this elsewhere: > > + If the optional WHERE clause is specified, only rows > + that satisfy the class="parameter">expression > + will be published. Note that parentheses are required around the > + expression. It has no effect on TRUNCATE commands. > > Maybe this whole thing is clearer if you just say "If the optional WHERE > clause is specified, rows for which the expression returns false or null > will not be published." With that it should be fairly clear what > happens if you have NULL values in the columns used in the expression, > and you can just delete that phrase you're discussing. > > Your proposal sounds good to me. Modified as suggested in v58 [1]. -- [1] https://www.postgresql.org/message-id/CAHut%2BPvkswkGLqzYo7z9rwOoDeLtUk0PEha8kppNvZts0h22Hw%40mail.gmail.com Kind Regards, Peter Smith. Fujitsu Australia
Re: row filtering for logical replication
On Thu, Dec 30, 2021 at 7:57 PM wangw.f...@fujitsu.com
wrote:
>
> On Mon, Dec 28, 2021 9:03 PM houzj.f...@fujitsu.com
> wrote:
> > Attach a top up patch 0004 which did the above changes.
>
> A few comments about v55-0001 and v55-0002.
...
> v55-0002
> In function pgoutput_row_filter_init, I found almost whole function is in the
> if
> statement written like this:
> static void
> pgoutput_row_filter_init()
> {
> Variable declaration and initialization;
> if (!entry->exprstate_valid)
> {
> ..
> }
> }
> What about changing this if statement like following:
> if (entry->exprstate_valid)
> return;
>
Modified in v58 [1] as suggested
--
[1]
https://www.postgresql.org/message-id/CAHut%2BPvkswkGLqzYo7z9rwOoDeLtUk0PEha8kppNvZts0h22Hw%40mail.gmail.com
Kind Regards,
Peter Smith.
Fujitsu Australia
Re: why does reindex invalidate relcache without modifying system tables
> 2021年12月27日 23:54，Tom Lane 写道： > > wenjing zeng writes: >> I found that in the index_update_stats function, i.e. the CREATE >> INDEX/REINDEX/Truncate INDEX process, >> relchche is invalidated whether the index information is updated. I want to >> know why you're did this > > Did you read the function's header comment? It says > > * NOTE: an important side-effect of this operation is that an SI invalidation > * message is sent out to all backends --- including me --- causing relcache > * entries to be flushed or updated with the new data. This must happen even > * if we find that no change is needed in the pg_class row. When updating > * a heap entry, this ensures that other backends find out about the new > * index. When updating an index, it's important because some index AMs > * expect a relcache flush to occur after REINDEX. > > That is, what we need to force an update of is either the relcache's > rd_indexlist list (for a table) or rd_amcache (for an index). > > In the REINDEX case, we could conceivably skip the flush on the table, > but not on the index. I don't think it's worth worrying about though, > because REINDEX will very probably have an update for the table's > physical size data (relpages and/or reltuples), so that it's unlikely > that the no-change path would be taken anyway. > > regards, tom lane Thank you for your explanation, which clears up my doubts. Wenjing
Re: Allow escape in application_name
On Tue, Jan 4, 2022 at 12:05 PM Kyotaro Horiguchi
wrote:
>
> At Wed, 29 Dec 2021 10:34:31 +0900, Masahiko Sawada
> wrote in
> > On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao
> > wrote:
> > >
> > >
> > >
> > > On 2021/12/28 9:32, Masahiko Sawada wrote:
> > > > Doesn't this query return 64? So the expression "substring(str for
> > > > (SELECT max_identifier_length FROM pg_control_init()))" returns the
> > > > first 64 characters of the given string while the application_name is
> > > > truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
> > > > fine to use current_setting('max_identifier_length') instead of
> > > > max_identifier_length of pg_control_init().
> > >
> > > Interesting! I agree that current_setting('max_identifier_length') should
> > > be used instead. Attached is the updated version of the patch. It uses
> > > current_setting('max_identifier_length').
> >
> > Thank you for updating the patch! It looks good to me.
>
> pg_terminate_backend returns just after kill() returns. So I'm afraid
> that there's a case where the next access to ft6 happens before the
> old connection actually ends on slow machines or under heavy load.
The test does pg_terminate_backend() with a timeout, and in this case,
don't we wait for the backend to end after sending the signal?
Regards,
--
Masahiko Sawada
EDB: https://www.enterprisedb.com/
Re: [HACKERS] logical decoding of two-phase transactions
Hi, On Mon, Aug 9, 2021 at 12:00 PM Amit Kapila wrote: > > On Wed, Aug 4, 2021 at 4:14 PM Amit Kapila wrote: > > > > I have pushed this last patch in the series. > > > > I have closed this CF entry. Thanks to everyone involved in this work! > I have a questoin about two_phase column of pg_replication_slots view: with this feature, pg_replication_slots has a new column two_phase: View "pg_catalog.pg_replication_slots" Column| Type | Collation | Nullable | Default -+-+---+--+- slot_name | name| | | plugin | name| | | slot_type | text| | | datoid | oid | | | database| name| | | temporary | boolean | | | active | boolean | | | active_pid | integer | | | xmin| xid | | | catalog_xmin| xid | | | restart_lsn | pg_lsn | | | confirmed_flush_lsn | pg_lsn | | | wal_status | text| | | safe_wal_size | bigint | | | two_phase | boolean | | | According to the doc, the two_phase field has: True if the slot is enabled for decoding prepared transactions. Always false for physical slots. It's unnatural a bit to me that replication slots have such a property since the replication slots have been used to protect WAL and tuples that are required for logical decoding, physical replication, and backup, etc from removal. Also, it seems that even if a replication slot is created with two_phase = off, it's overwritten to on if the plugin enables two-phase option. Is there any reason why we can turn on and off this value on the replication slot side and is there any use case where the replication slot’s two_phase is false and the plugin’s two-phase option is on and vice versa? I think that we can have replication slots always have two_phase_at value and remove the two_phase field from the view. Regards, -- Masahiko Sawada EDB: https://www.enterprisedb.com/
Re: Allow escape in application_name
At Wed, 29 Dec 2021 10:34:31 +0900, Masahiko Sawada
wrote in
> On Tue, Dec 28, 2021 at 3:29 PM Fujii Masao
> wrote:
> >
> >
> >
> > On 2021/12/28 9:32, Masahiko Sawada wrote:
> > > Doesn't this query return 64? So the expression "substring(str for
> > > (SELECT max_identifier_length FROM pg_control_init()))" returns the
> > > first 64 characters of the given string while the application_name is
> > > truncated to be 63 (NAMEDATALEN - 1) characters. It also seems to be
> > > fine to use current_setting('max_identifier_length') instead of
> > > max_identifier_length of pg_control_init().
> >
> > Interesting! I agree that current_setting('max_identifier_length') should
> > be used instead. Attached is the updated version of the patch. It uses
> > current_setting('max_identifier_length').
>
> Thank you for updating the patch! It looks good to me.
pg_terminate_backend returns just after kill() returns. So I'm afraid
that there's a case where the next access to ft6 happens before the
old connection actually ends on slow machines or under heavy load.
> > BTW it seems confusing that pg_control_init() (also pg_controldata) and GUC
> > report different values as max_identifier_length. Probably they should
> > return the same value such as NAMEDATALEN - 1. But this change might be
> > overkill...
>
> Agreed. Probably we can fix it in a separate patch if necessary.
Agree to another patch, but I think we should at least add a caution
that they are different. I'm not sure we can change the context of
ControlFileData.nameDataLen.
regards.
--
Kyotaro Horiguchi
NTT Open Source Software Center
Re: ICU for global collation
Hi,
On Thu, Dec 30, 2021 at 01:07:21PM +0100, Peter Eisentraut wrote:
>
> So this is a different approach: If you choose ICU as the default locale for
> a database, you still need to specify lc_ctype and lc_collate settings, as
> before. Unlike in the previous patch, where the ICU collation name was
> written in datcollate, there is now a third column (daticucoll), so we can
> store all three values. This fixes the described problem. Other than that,
> once you get all the initial settings right, it basically just works: The
> places that have ICU support now will use a database-wide ICU collation if
> appropriate, the places that don't have ICU support continue to use the
> global libc locale settings.
That looks sensible to me.
> @@ -2774,6 +2776,7 @@ dumpDatabase(Archive *fout)
> appendPQExpBuffer(dbQry, "SELECT tableoid, oid, datname, "
> "(%s datdba) AS dba, "
>
> "pg_encoding_to_char(encoding) AS encoding, "
> + "datcollprovider, "
This needs to be in a new pg 15+ branch, not in the pg 9.3+.
> - if (!lc_collate_is_c(collid) && collid != DEFAULT_COLLATION_OID)
> - mylocale = pg_newlocale_from_collation(collid);
> + if (!lc_collate_is_c(collid))
> + {
> + if (collid != DEFAULT_COLLATION_OID)
> + mylocale = pg_newlocale_from_collation(collid);
> + else if (default_locale.provider == COLLPROVIDER_ICU)
> + mylocale = _locale;
> + }
There are really a lot of places with this new code. Maybe it could be some
new function/macro to wrap that for the normal case (e.g. not formatting.c)?
Re: Add checkpoint and redo LSN to LogCheckpointEnd log message
At Tue, 28 Dec 2021 08:26:28 +0530, Bharath Rupireddy wrote in > On Fri, Dec 24, 2021 at 5:54 PM Bharath Rupireddy > wrote: > > > > On Fri, Dec 24, 2021 at 5:42 PM Michael Paquier wrote: > > > > > > On Fri, Dec 24, 2021 at 02:51:34PM +0900, Kyotaro Horiguchi wrote: > > > > I thougt about something like the following, but your proposal may be > > > > clearer. > > > > > > +"LSN=%X/%X, REDO LSN=%X/%X", > > > This could be rather confusing for the average user, even if I agree > > > that this is some information that only an advanced user could > > > understand. Could it be possible to define those fields in a more > > > deterministic way? For one, it is hard to understand the relationship > > > between both fields without looking at the code, particulary if both > > > share the same value. It is at least rather.. Well, mostly, easy to > > > guess what each other field means in this context, which is not the > > > case of what you are proposing here. One idea could be use also > > > "start point" for REDO, for example. > > > > How about "location=%X/%X, REDO start location=%X/%X"? The entire log > > message can look like below: .. > Thoughts? It seems to me "LSN" or just "location" is more confusing or mysterious than "REDO LSN" for the average user. If we want to avoid being technically too detailed, we would use just "start LSN=%X/%X, end LSN=%X/%X". And it is equivalent to "WAL range=[%X/%X, %X/%X]".. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: pg_stat_bgwriter.buffers_backend is pretty meaningless (and more?)
On Thu, Dec 30, 2021 at 3:30 PM Melanie Plageman wrote: > > On Tue, Dec 21, 2021 at 8:32 PM Melanie Plageman > wrote: > > On Thu, Dec 16, 2021 at 3:18 PM Andres Freund wrote: > > > > > > From 9f22da9041e1e1fbc0ef003f5f78f4e72274d438 Mon Sep 17 00:00:00 > > > > > > 2001 > > > > > > From: Melanie Plageman > > > > > > Date: Wed, 24 Nov 2021 12:20:10 -0500 > > > > > > Subject: [PATCH v17 6/7] Remove superfluous bgwriter stats > > > > > > > > > > > > Remove stats from pg_stat_bgwriter which are now more clearly > > > > > > expressed > > > > > > in pg_stat_buffers. > > > > > > > > > > > > TODO: > > > > > > - make pg_stat_checkpointer view and move relevant stats into it > > > > > > - add additional stats to pg_stat_bgwriter > > > > > > > > > > When do you think it makes sense to tackle these wrt committing some > > > > > of the > > > > > patches? > > > > > > > > Well, the new stats are a superset of the old stats (no stats have been > > > > removed that are not represented in the new or old views). So, I don't > > > > see that as a blocker for committing these patches. > > > > > > > Since it is weird that pg_stat_bgwriter had mostly checkpointer stats, > > > > I've edited this commit to rename that view to pg_stat_checkpointer. > > > > > > > I have not made a separate view just for maxwritten_clean (presumably > > > > called pg_stat_bgwriter), but I would not be opposed to doing this if > > > > you thought having a view with a single column isn't a problem (in the > > > > event that we don't get around to adding more bgwriter stats right > > > > away). > > > > > > How about keeping old bgwriter values in place in the view , but generated > > > from the new stats stuff? > > > > I tried this, but I actually don't think it is the right way to go. In > > order to maintain the old view with the new source code, I had to add > > new code to maintain a separate resets array just for the bgwriter view. > > It adds some fiddly code that will be annoying to maintain (the reset > > logic is confusing enough as is). > > And, besides the implementation complexity, if a user resets > > pg_stat_bgwriter and not pg_stat_buffers (or vice versa), they will > > see totally different numbers for "buffers_backend" in pg_stat_bgwriter > > than shared buffers written by B_BACKEND in pg_stat_buffers. I would > > find that confusing. > > In a quick chat off-list, Andres suggested it might be okay to have a > single reset target for both the pg_stat_buffers view and legacy > pg_stat_bgwriter view. So, I am planning to share a new patchset which > has only the new "buffers" target which will also reset the legacy > pg_stat_bgwriter view. > > I'll also remove the bgwriter stats I proposed and the > pg_stat_checkpointer view to keep things simple for now. > I've done the above in v20, attached. - Melanie From 14e20657d6a6674cc286b5ce1a29560889cf7833 Mon Sep 17 00:00:00 2001 From: Melanie Plageman Date: Wed, 24 Nov 2021 12:07:37 -0500 Subject: [PATCH v20 6/8] Add system view tracking IO ops per backend type Add pg_stat_buffers, a system view which tracks the number of IO operations (allocs, writes, fsyncs, and extends) done through each IO path (e.g. shared buffers, local buffers, unbuffered IO) by each type of backend. Some of these should always be zero. For example, checkpointer does not use a BufferAccessStrategy (currently), so the "strategy" IO Path for checkpointer will be 0 for all IO operations (alloc, write, fsync, and extend). All possible combinations of IOPath and IOOp are enumerated in the view but not all are populated or even possible at this point. All backends increment a counter in an array of IO stat counters in their PgBackendStatus when performing an IO operation. On exit, backends send these stats to the stats collector to be persisted. When the pg_stat_buffers view is queried, one backend will sum live backends' stats with saved stats from exited backends and subtract saved reset stats, returning the total. Each row of the view is stats for a particular backend type for a particular IO Path (e.g. shared buffer accesses by checkpointer) and each column in the view is the total number of IO operations done (e.g. writes). So a cell in the view would be, for example, the number of shared buffers written by checkpointer since the last stats reset. Suggested by Andres Freund Author: Melanie Plageman Reviewed-by: Justin Pryzby Discussion: https://www.postgresql.org/message-id/flat/20200124195226.lth52iydq2n2uilq%40alap3.anarazel.de --- doc/src/sgml/monitoring.sgml| 119 +++- src/backend/catalog/system_views.sql| 11 ++ src/backend/postmaster/pgstat.c | 13 ++ src/backend/utils/activity/backend_status.c | 19 ++- src/backend/utils/adt/pgstatfuncs.c | 150 src/include/catalog/pg_proc.dat | 9 ++ src/include/pgstat.h| 1 + src/include/utils/backend_status.h | 1 +
Re: more descriptive message for process termination due to max_slot_wal_keep_size
At Fri, 24 Dec 2021 20:23:29 +0900, Masahiko Sawada wrote in > On Fri, Dec 24, 2021 at 5:30 PM Kyotaro Horiguchi > wrote: > > Right. But I believe the two are not assumed to be used at once. One > > can set wal_keep_size larger than max_slot_wal_keep_size but it is > > actually a kind of ill setting. > > > > LOG: terminating process %d to release replication slot \"%s\" because its > > restart_lsn %X/%X exceeds max_slot_wal_keep_size > > DETAIL: The slot got behind the limit %X/%X determined by > > max_slot_wal_keep_size and wal_keep_size. > > > > Mmm. I don't like this. I feel we don't need such detail in the > > message. > > How about something like: > > LOG: terminating process %d to release replication slot \"%s\" > because its restart_lsn %X/%X exceeds the limit > DETAIL: The slot got behind the limit %X/%X > HINT: You might need to increase max_slot_wal_keep_size or wal_keep_size. The message won't be seen when max_slot_wal_keep_size is not set. So we don't recommend to increase wal_keep_size in that case. We might need inhibit (or warn)the two parameters from being activated at once, but it would be another issue. Another point is how people determine the value for the parameter. I suppose (or believe) max_slot_wal_keep_size is not a kind to set to minimal first then increase later but a kind to set to maximum allowable first. On the other hand we suggest as the follows for too-small max_wal_size so we could do the same for this parameter. > HINT: Consider increasing the configuration parameter \"max_wal_size\". Also, I don't like we have three lines for this message. If the DETAIL adds only the specific value of the limit, I think it'd better append it to the main message. So what do you say if I propose the following? LOG: terminating process %d to release replication slot \"%s\" because its restart_lsn %X/%X exceeds the limit %X/%X HINT: You might need to increase max_slot_wal_keep_size. regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: An obsolete comment of pg_stat_statements
At Mon, 3 Jan 2022 17:36:25 +0900, Michael Paquier wrote in > On Fri, Dec 24, 2021 at 09:02:10PM +0900, Michael Paquier wrote: > > Do we really need to have this comment in the function header? The > > same is explained a couple of lines down so this feels like a > > duplicate, and it is hard to miss it with the code shaped as-is (aka > > the relationship between compute_query_id and queryId and the > > consequences on what's stored in this case). > > The simpler the better here. So, I have just removed this comment > after thinking more about this. I'm fine with it. Thanks! regards. -- Kyotaro Horiguchi NTT Open Source Software Center
Re: [PoC] Delegating pg_ident to a third party
Greetings, * Jacob Champion (pchamp...@vmware.com) wrote: > On Mon, 2022-01-03 at 12:36 -0500, Stephen Frost wrote: > > * Jacob Champion (pchamp...@vmware.com) wrote: > > > On Fri, 2021-12-17 at 10:06 +0100, Peter Eisentraut wrote: > > > > On 17.12.21 00:48, Jacob Champion wrote: > > > > > WDYT? (My responses here will be slower than usual. Hope you all have > > > > > a > > > > > great end to the year!) > > > > > > > > Looks interesting. I wonder whether putting this into pg_ident.conf is > > > > sensible. I suspect people will want to eventually add more features > > > > around this, like automatically creating roles or role memberships, at > > > > which point pg_ident.conf doesn't seem appropriate anymore. > > > > This is the part that I really wonder about also ... I've always viewed > > pg_ident as being intended mainly for one-to-one kind of mappings and > > not the "map a bunch of different users into the same role" that this > > advocated for. Being able to have roles and memberships automatically > > created is much more the direction that I'd say we should be going in, > > so that in-database auditing has an actual user to go on and not some > > generic role that could be any number of people. > > That last point was my motivation for the authn_id patch [1] -- so that > auditing could see the actual user _and_ the generic role. The > information is already there to be used, it's just not exposed to the > stats framework yet. While that helps, and I generally support adding that information to the logs, it's certainly not nearly as good or useful as having the actual user known to the database. > Forcing one role per individual end user is wasteful and isn't really > making good use of the role-based system that you already have. > Generally speaking, when administering hundreds or thousands of users, > people start dividing them up into groups as opposed to dealing with > them individually. So I don't think new features should be taking away > flexibility in this area -- if one role per user already works well for > you, great, but don't make everyone do the same. Using the role system we have to assign privileges certainly is useful and sensible, of course, though I don't see where you've actually made an argument for why one role per individual is somehow wasteful or somehow takes away from the role system that we have for granting rights. I'm also not suggesting that we make everyone do the same thing, indeed, later on I was supportive of having an external system provide the mapping. Here, I'm just making the point that we should also be looking at automatic role/membership creation. > > I'd go a step further and suggest that the way to do this is with a > > background worker that's started up and connects to an LDAP > > infrastructure and listens for changes, allowing the system to pick up > > on new roles/memberships as soon as they're created in the LDAP > > environment. That would then be controlled by appropriate settings in > > postgresql.conf/.auto.conf. > > This is roughly what you can already do with existing (third-party) > tools, and that approach isn't scaling out in practice for some of our > existing customers. The load on the central server, for thousands of > idle databases dialing in just to see if there are any new users, is > huge. If you're referring specifically to cron-based tools which are constantly hammering on the LDAP servers running the same queries over and over, sure, I agree that that's creating load on the LDAP infrastructure (though, well, it was kind of designed to be very scalable for exactly that kind of load, no? So I'm not really sure why that's such an issue..). That's also why I specifically wasn't suggesting that and was instead suggesting that we have something that's connected to one of the (hopefully, many, many) LDAP servers and is doing change monitoring, allowing changes to be pushed down to PG, rather than cronjobs constantly running the same queries and re-checking things over and over. I appreciate that that's also not free, but I don't believe it's nearly as bad as the cron-based approach and it's certainly something that an LDAP infrastructure should be really rather good at. > > All that said, I do see how having the ability to call out to another > > system for mappings may be useful, so I'm not sure that we shouldn't > > consider this specific change and have it be specifically just for > > mappings, in which case pg_ident seems appropriate. > > Yeah, this PoC was mostly an increment on the functionality that > already existed. The division between what goes in pg_hba and what goes > in pg_ident is starting to blur with this patchset, though, and I think > Peter's point is sound. This part I tend to disagree with- pg_ident for mappings and for ways to call out to other systems to provide those mappings strikes me as entirely appropriate and doesn't blur the lines and that's really what this patch seems to be
Re: What does RIR as in fireRIRrules stand for?
fred yin writes: > I found this thread when trying to understand what RIR means in the source > code. The explanation by Andres (which I find informative) is not in the > code. Should we add it into the comment? I think it will be useful for > readability. There's this at the top of rewriteHandler.c: * NOTES *Some of the terms used in this file are of historic nature: "retrieve" *was the PostQUEL keyword for what today is SELECT. "RIR" stands for *"Retrieve-Instead-Retrieve", that is an ON SELECT DO INSTEAD SELECT rule *(which has to be unconditional and where only one rule can exist on each *relation). regards, tom lane
Re: What does RIR as in fireRIRrules stand for?
I found this thread when trying to understand what RIR means in the source code. The explanation by Andres (which I find informative) is not in the code. Should we add it into the comment? I think it will be useful for readability. Best, Yue On Mon, Jan 3, 2022 at 5:26 PM Andres Freund wrote: > On 2015-08-27 15:13:52 +0200, Andres Freund wrote: > > I searched the current code, all diffs to the current code, and the > > mailing list, but still haven't got an actual clue what RIR is supposed > > to stand for. There's a few things that come to mind (Rewrite Instead > > Rule?), but none of them seem to make too much sense. > > Looking over the original submission it seems to actually stand for > retrieve-instead-retrieve (as somebody on IRC guessed). Doesn't seem to > make much sense to me. > > The explanation seems to have been removed way back, in f93b6974 . > > Greetings, > > Andres Freund > > > -- > Sent via pgsql-hackers mailing list (pgsql-hack...@postgresql.org) > To make changes to your subscription: > http://www.postgresql.org/mailpref/pgsql-hackers > > >
Re: Bug fix for tab completion of ALTER TABLE ... VALIDATE CONSTRAINT ...
Aleksander Alekseev writes: > The cfbot seems to be happy with the updated patch. > The new status of this patch is: Ready for Committer Pushed. regards, tom lane
Re: Accessing fields past CATALOG_VARLEN
Ed Behn writes: > I can get fields like procStruct->prokind and procStruct->proretset. > However, I get a compiler error when I try to access procStruct->proargmodes. > I know that this is because this field is in the CATALOG_VARLEN block which > makes it invisible to the compiler. > What is the proper way to get this field? SysCacheGetAttr(). There are examples all over the tree, but one that's specific to proargmodes (and also illustrates the best practices for deciphering its value) is in parser/analyze.c's transformCallStmt(). You should also ask yourself if you really *need* to examine proargmodes for yourself, or if there's a utility function somewhere that will compute what you need to know. regards, tom lane
Re: Accessing fields past CATALOG_VARLEN
On 01/03/22 17:23, Ed Behn wrote: > However, I get a compiler error when I try to access procStruct->proargmodes. > I know that this is because this field is in the CATALOG_VARLEN block which > makes it invisible to the compiler. > > What is the proper way to get this field? You can use SysCacheGetAttr with the attribute number. It knows all the magic needed to find the right offset, possibly decompress, etc. Regards, -Chap
Re: SKIP LOCKED assert triggered
On 2021-Dec-01, Simon Riggs wrote: > On Wed, 1 Dec 2021 at 14:33, Bossart, Nathan wrote: > > > > On 11/12/21, 8:56 AM, "Simon Riggs" wrote: > > > The combination of these two statements in a transaction hits an > > > Assert in heapam.c at line 4770 on REL_14_STABLE > > > > I've been unable to reproduce this. Do you have any tips for how to > > do so? Does there need to be some sort of concurrent workload? > > That path is only ever taken when there are multiple sessions, and as > I said, pgbench finds this reliably. I guess I didn't say "use -c 2" Simon had sent me the pgbench scripts earlier, so I attach them here. I don't actually get a crash with -c2 or -c3, but I do get almost immediate crashes with -c4 and above. If I run it under "rr", it doesn't occur either. I suspect the rr framework kills concurrency in some way that hides the problem. I didn't find a way to reproduce it with isolationtester. (If somebody wants to play with a debugger, I find that it's much easier to reproduce by adding a short sleep after the UpdateXmaxHintBits() call in line 4735; but that sleep occurs in a session *other* than the one that dies. And under rr I still don't see a crash with a sleep there; in fact the sleep doesn't seem to occur at all, which is weird.) The patch does fix the crasher under pgbench, and I think it makes sense that you can get WouldBlock and yet have the tuple marked with XMAX_INVALID: if transaction A is writing the tuple, and transaction B is acquiring the tuple lock, then transaction C also tries to acquire the tuple lock but that returns nay (because of B), then transaction A completes, then transaction B could set the XMAX_INVALID flag in time for C to have a seizure in its way out. So patching the assertion to allow the case is correct. What I don't understand is why hasn't this been reported already: this bug is pretty old. My only explanation is that nobody runs sufficiently- concurrent load with SKIP LOCKED in assert-enabled builds. [1] https://www.postgresql.org/message-id/flat/CADLWmXUvd5Z%2BcFczi6Zj1WcTrXzipgP-wj0pZOWSaRUy%3DF0omQ%40mail.gmail.com -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ simon_setup.sql Description: application/sql simon_bench.sql Description: application/sql
Accessing fields past CATALOG_VARLEN
I'm trying to write a C-language function to be compiled into a
shared module to be loaded by Postgres. In it, I have the OID of a function
and I need to get information from the pg_proc table.
So far, I have:
HeapTuple procTuple;
> Form_pg_proc procStruct;
> procTuple = SearchSysCache1(PROCOID, ObjectIdGetDatum(funcoid));
> if(!HeapTupleIsValid(procTuple))
> ereport(ERROR, errmsg("cache lookup failed for function %u.",
> funcoid));
>
procStruct = (Form_pg_proc) GETSTRUCT(procTuple);
I can get fields like procStruct->prokind and procStruct->proretset.
However, I get a compiler error when I try to access procStruct->proargmodes.
I know that this is because this field is in the CATALOG_VARLEN block which
makes it invisible to the compiler.
What is the proper way to get this field?
-Ed
Re: CREATEROLE and role ownership hierarchies
On 12/23/21 16:06, Joshua Brindle wrote: > On Tue, Dec 21, 2021 at 8:26 PM Mark Dilger > wrote: >> >> >>> On Dec 21, 2021, at 5:11 PM, Shinya Kato >>> wrote: >>> >>> I fixed the patches because they cannot be applied to HEAD. >> Thank you. > I reviewed and tested these and they LGTM. FYI the rebased v3 patches > upthread are raw diffs so git am won't apply them. That's not at all unusual. I normally apply patches just using patch -p 1 < $patchfile > I can add myself to > the CF as a reviewer if it is helpful. Please do. cheers andrew -- Andrew Dunstan EDB: https://www.enterprisedb.com
Re: PostgreSQL stops when adding a breakpoint in CLion
Stanislav Bashkyrtsev writes:
>> Why do you think postgres quits?
> The process was running and then it stopped. And in the console I see:
> 2022-01-03 23:23:29.495 MSK [76717] LOG: checkpoint starting: shutdown
> immediate
In a standalone backend, I think there are only 3 ways to get to
normal shutdown:
* SIGTERM
* SIGQUIT
* EOF on stdin
It's not very clear which of those your setup is triggering.
In any case, debugging standalone mode is very very rarely
what you should be doing; it's only vaguely related to normal
operation, plus you lack all the creature comforts of psql.
The usual thing is to start a normal interactive session, find out
the PID of its connected backend process ("select pg_backend_pid()"
is a reliable way), and then attach to that process with GDB or your
debugger of choice.
regards, tom lane
Proposal: remove obsolete hot-standby testing infrastructure
The attached proposed patch removes some ancient infrastructure for manually testing hot standby. I doubt anyone has used this in years, because AFAICS there is nothing here that's not done better by the src/test/recovery TAP tests. (Or if there is, we ought to migrate it into the TAP tests.) Thoughts? regards, tom lane diff --git a/doc/src/sgml/regress.sgml b/doc/src/sgml/regress.sgml index 724ef566e7..8cf10085d3 100644 --- a/doc/src/sgml/regress.sgml +++ b/doc/src/sgml/regress.sgml @@ -391,52 +391,6 @@ make check EXTRA_TESTS=numeric_big - - - Testing Hot Standby - - - The source distribution also contains regression tests for the static - behavior of Hot Standby. These tests require a running primary server - and a running standby server that is accepting new WAL changes from the - primary (using either file-based log shipping or streaming replication). - Those servers are not automatically created for you, nor is replication - setup documented here. Please check the various sections of the - documentation devoted to the required commands and related issues. - - - - To run the Hot Standby tests, first create a database - called regression on the primary: - -psql -h primary -c "CREATE DATABASE regression" - - Next, run the preparatory script - src/test/regress/sql/hs_primary_setup.sql - on the primary in the regression database, for example: - -psql -h primary -f src/test/regress/sql/hs_primary_setup.sql regression - - Allow these changes to propagate to the standby. - - - - Now arrange for the default database connection to be to the standby - server under test (for example, by setting the PGHOST and - PGPORT environment variables). - Finally, run make standbycheck in the regression directory: - -cd src/test/regress -make standbycheck - - - - - Some extreme behaviors can also be generated on the primary using the - script src/test/regress/sql/hs_primary_extremes.sql - to allow the behavior of the standby to be tested. - - diff --git a/src/test/regress/GNUmakefile b/src/test/regress/GNUmakefile index 330eca2b83..1ef45b82ec 100644 --- a/src/test/regress/GNUmakefile +++ b/src/test/regress/GNUmakefile @@ -130,9 +130,6 @@ installcheck-parallel: all installcheck-tests: all $(pg_regress_installcheck) $(REGRESS_OPTS) $(TESTS) $(EXTRA_TESTS) -standbycheck: all - $(pg_regress_installcheck) $(REGRESS_OPTS) --schedule=$(srcdir)/standby_schedule --use-existing - # old interfaces follow... runcheck: check diff --git a/src/test/regress/expected/hs_standby_allowed.out b/src/test/regress/expected/hs_standby_allowed.out deleted file mode 100644 index 00b8faf9eb..00 --- a/src/test/regress/expected/hs_standby_allowed.out +++ /dev/null @@ -1,218 +0,0 @@ --- --- Hot Standby tests --- --- hs_standby_allowed.sql --- --- SELECT -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -select count(*) as should_be_2 from hs2; - should_be_2 -- - 2 -(1 row) - -select count(*) as should_be_3 from hs3; - should_be_3 -- - 3 -(1 row) - -COPY hs1 TO '/tmp/copy_test'; -\! cat /tmp/copy_test -1 --- Access sequence directly -select is_called from hsseq; - is_called - f -(1 row) - --- Transactions -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -end; -begin transaction read only; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -end; -begin transaction isolation level repeatable read; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -commit; -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -commit; -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -abort; -start transaction; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -commit; -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -rollback; -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -savepoint s; -select count(*) as should_be_2 from hs2; - should_be_2 -- - 2 -(1 row) - -commit; -begin; -select count(*) as should_be_1 from hs1; - should_be_1 -- - 1 -(1 row) - -savepoint s; -select count(*) as should_be_2 from hs2; - should_be_2 -- - 2 -(1 row) - -release savepoint s; -select count(*) as should_be_2 from hs2; - should_be_2 -- - 2 -(1 row) - -savepoint s; -select
Re: pg_dump/restore --no-tableam
@cfbot: rebased
>From 69ae2ed5d00a97d351e1f6c45a9e406f33032898 Mon Sep 17 00:00:00 2001
From: Justin Pryzby
Date: Sun, 7 Mar 2021 19:35:37 -0600
Subject: [PATCH] Add pg_dump/restore --no-table-am..
This was for some reason omitted from 3b925e905.
---
doc/src/sgml/ref/pg_dump.sgml| 18 +++
doc/src/sgml/ref/pg_restore.sgml | 11 +
src/bin/pg_dump/pg_backup.h | 2 ++
src/bin/pg_dump/pg_backup_archiver.c | 12 ++
src/bin/pg_dump/pg_dump.c| 3 +++
src/bin/pg_dump/pg_restore.c | 4
src/bin/pg_dump/t/002_pg_dump.pl | 34 ++--
7 files changed, 72 insertions(+), 12 deletions(-)
diff --git a/doc/src/sgml/ref/pg_dump.sgml b/doc/src/sgml/ref/pg_dump.sgml
index 0e1cfe0f8d6..edfeab50b8e 100644
--- a/doc/src/sgml/ref/pg_dump.sgml
+++ b/doc/src/sgml/ref/pg_dump.sgml
@@ -931,6 +931,24 @@ PostgreSQL documentation
+
+
+ --no-table-am
+
+
+Do not output commands to select table access methods.
+With this option, all objects will be created with whichever
+table access method is the default during restore.
+
+
+
+This option is ignored when emitting an archive (non-text) output
+file. For the archive formats, you can specify the option when you
+call pg_restore.
+
+
+
+
--no-tablespaces
diff --git a/doc/src/sgml/ref/pg_restore.sgml b/doc/src/sgml/ref/pg_restore.sgml
index 93ea937ac8e..a68f3a85b7e 100644
--- a/doc/src/sgml/ref/pg_restore.sgml
+++ b/doc/src/sgml/ref/pg_restore.sgml
@@ -649,6 +649,17 @@ PostgreSQL documentation
+
+ --no-table-am
+
+
+Do not output commands to select table access methods.
+With this option, all objects will be created with whichever
+access method is the default during restore.
+
+
+
+
--no-tablespaces
diff --git a/src/bin/pg_dump/pg_backup.h b/src/bin/pg_dump/pg_backup.h
index 753252e05e0..47741ad6406 100644
--- a/src/bin/pg_dump/pg_backup.h
+++ b/src/bin/pg_dump/pg_backup.h
@@ -93,6 +93,7 @@ typedef struct _restoreOptions
{
int createDB; /* Issue commands to create the database */
int noOwner; /* Don't try to match original object owner */
+ int noTableAm; /* Don't issue tableAM-related commands */
int noTablespace; /* Don't issue tablespace-related commands */
int disable_triggers; /* disable triggers during data-only
* restore */
@@ -179,6 +180,7 @@ typedef struct _dumpOptions
int no_unlogged_table_data;
int serializable_deferrable;
int disable_triggers;
+ int outputNoTableAm;
int outputNoTablespaces;
int use_setsessauth;
int enable_row_security;
diff --git a/src/bin/pg_dump/pg_backup_archiver.c b/src/bin/pg_dump/pg_backup_archiver.c
index 8903a694ae9..0f62beac0fc 100644
--- a/src/bin/pg_dump/pg_backup_archiver.c
+++ b/src/bin/pg_dump/pg_backup_archiver.c
@@ -194,6 +194,7 @@ dumpOptionsFromRestoreOptions(RestoreOptions *ropt)
dopt->outputSuperuser = ropt->superuser;
dopt->outputCreateDB = ropt->createDB;
dopt->outputNoOwner = ropt->noOwner;
+ dopt->outputNoTableAm = ropt->noTableAm;
dopt->outputNoTablespaces = ropt->noTablespace;
dopt->disable_triggers = ropt->disable_triggers;
dopt->use_setsessauth = ropt->use_setsessauth;
@@ -3171,6 +3172,11 @@ _reconnectToDB(ArchiveHandle *AH, const char *dbname)
if (AH->currSchema)
free(AH->currSchema);
AH->currSchema = NULL;
+
+ if (AH->currTableAm)
+ free(AH->currTableAm);
+ AH->currTableAm = NULL;
+
if (AH->currTablespace)
free(AH->currTablespace);
AH->currTablespace = NULL;
@@ -3340,10 +3346,15 @@ _selectTablespace(ArchiveHandle *AH, const char *tablespace)
static void
_selectTableAccessMethod(ArchiveHandle *AH, const char *tableam)
{
+ RestoreOptions *ropt = AH->public.ropt;
PQExpBuffer cmd;
const char *want,
*have;
+ /* do nothing in --no-table-am mode */
+ if (ropt->noTableAm)
+ return;
+
have = AH->currTableAm;
want = tableam;
@@ -4770,6 +4781,7 @@ CloneArchive(ArchiveHandle *AH)
clone->connCancel = NULL;
clone->currUser = NULL;
clone->currSchema = NULL;
+ clone->currTableAm = NULL;
clone->currTablespace = NULL;
/* savedPassword must be local in case we change it while connecting */
diff --git a/src/bin/pg_dump/pg_dump.c b/src/bin/pg_dump/pg_dump.c
index 7af6dfa5759..532e721a695 100644
--- a/src/bin/pg_dump/pg_dump.c
+++ b/src/bin/pg_dump/pg_dump.c
@@ -389,6 +389,7 @@ main(int argc, char **argv)
{"if-exists", no_argument, _exists, 1},
{"inserts", no_argument, NULL, 9},
{"lock-wait-timeout", required_argument, NULL, 2},
+ {"no-table-am", no_argument, , 1},
{"no-tablespaces", no_argument, , 1},
{"quote-all-identifiers", no_argument, _all_identifiers, 1},
{"load-via-partition-root", no_argument, _via_partition_root, 1},
@@ -933,6 +934,7 @@ main(int argc, char
Re: PostgreSQL stops when adding a breakpoint in CLion
> Why do you think postgres quits? The process was running and then it stopped. And in the console I see: 2022-01-03 23:23:29.495 MSK [76717] LOG: checkpoint starting: shutdown immediate 2022-01-03 23:23:29.498 MSK [76717] LOG: checkpoint complete: wrote 3 buffers (0.0%); 0 WAL file(s) added, 0 removed, 0 recycled; write=0.002 s, sync=0.001 s, total=0.005 s; sync files=2, longest=0.001 s, average=0.001 s; distance=0 kB, estimate=0 kB > AFAIK CLion uses gdb or lldb for > debugging, which are the debugger of choice for many (most?) hackers on > this list. So that should work fine. Yep, and it worked for me too.. Yesterday :) I see that CLion uses LLDB on MacOS by default. > Now sure why you start it in single-user mode, but I don't think that > should affect debugging. Well, --single seems convenient because CLion starts that process and attaches to it right away. I don't have to look for a way of attaching to the forks. Maybe it's a good point to mention that I'm not very familiar with developing in C/C++ and therefore have a vague understanding of how to set up an efficient dev environment. Moreover in multi-user mode CLion/LLDB keeps stopping in postmaster.c: selres = select(nSockets, , NULL, NULL, ); >Try redirecting the output to a log file, maybe > that'll tell you what happened. I see all the output in the console, so not sure what redirecting to a file would achieve. On Mon, Jan 3, 2022 at 10:08 PM Tom Lane wrote: > Tomas Vondra writes: > > On 1/3/22 16:54, Stanislav Bashkyrtsev wrote: > >> - If I put a breakpoint before I start the process then everything > works > >> fine > >> - But if I put/remove a breakpoint after it's fully initialized - the > >> process just stops > > > Why do you think postgres quits? AFAIK CLion uses gdb or lldb for > > debugging, which are the debugger of choice for many (most?) hackers on > > this list. So that should work fine. > > FWIW, it's normal in gdb that if you attach to an existing process, > the process stops until you say "continue". I know nothing of CLion, > but it likely follows that convention too. > > regards, tom lane >
Re: [PATCH] pg_stat_toast v6
Overall I think this is a good feature to have; assessing the need for compression is important for tuning, so +1 for the goal of the patch. I didn't look into the patch carefully, but here are some minor comments: On 2022-Jan-03, Gunnar "Nick" Bluth wrote: > @@ -229,7 +230,9 @@ toast_tuple_try_compression(ToastTupleContext *ttc, int > attribute) > Datum *value = >ttc_values[attribute]; > Datum new_value; > ToastAttrInfo *attr = >ttc_attr[attribute]; > + instr_time start_time; > > + INSTR_TIME_SET_CURRENT(start_time); > new_value = toast_compress_datum(*value, attr->tai_compression); > > if (DatumGetPointer(new_value) != NULL) Don't INSTR_TIME_SET_CURRENT unconditionally; in some systems it's an expensive syscall. Find a way to only do it if the feature is enabled. This also suggests that perhaps it'd be a good idea to allow this to be enabled for specific tables only, rather than system-wide. (Maybe in order for stats to be collected, the user should have to both set the GUC option *and* set a per-table option? Not sure.) > @@ -82,10 +82,12 @@ typedef enum StatMsgType > PGSTAT_MTYPE_DEADLOCK, > PGSTAT_MTYPE_CHECKSUMFAILURE, > PGSTAT_MTYPE_REPLSLOT, > + PGSTAT_MTYPE_CONNECTION, I think this new enum value doesn't belong in this patch. > +/* -- > + * PgStat_ToastEntry Per-TOAST-column info in a MsgFuncstat > + * -- Not in "a MsgFuncstat", right? > +-- function to wait for counters to advance > +create function wait_for_stats() returns void as $$ I don't think we want a separate copy of wait_for_stats; see commit fe60b67250a3 and the discussion leading to it. Maybe you'll want to move the test to stats.sql. I'm not sure what to say about relying on LZ4; maybe you'll want to leave that part out, and just verify in an LZ4-enabled build that some 'l' entry exists. BTW, don't we have any decent way to turn that 'l' into a more reasonable, descriptive string? Also, perhaps make the view-defining query turn an empty compression method into whatever the default is. Or even better, stats collection should store the real compression method used rather than empty string, to avoid confusing things when some stats are collected, then the default is changed, then some more stats are collected. -- Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/ "Para tener más hay que desear menos"
Re: [PATCH] pg_stat_toast v6
> +pgstat_report_toast_activity(Oid relid, int attr,
> + bool externalized,
> + bool compressed,
> + int32 old_size,
> + int32 new_size,
...
> + if (new_size)
> + {
> + htabent->t_counts.t_size_orig+=old_size;
> + if (new_size)
> + {
I guess one of these is supposed to say old_size?
> + _track_toast,
> + false,
> + NULL, NULL, NULL
> + },
> {
> +CREATE TABLE toast_test (cola TEXT, colb TEXT COMPRESSION lz4, colc TEXT ,
> cold TEXT, cole TEXT);
Is there a reason this uses lz4 ?
If that's needed for stable results, I think you should use pglz, since that's
what's guaranteed to exist. I imagine LZ4 won't be required any time soon,
seeing as zlib has never been required.
> +Be aware that this feature, depending on the amount of TOASTable
> columns in
> +your databases, may significantly increase the size of the
> statistics files
> +and the workload of the statistics collector. It is recommended to
> only
> +temporarily activate this to assess the right compression and
> storage method
> +for (a) column(s).
saying "a column" is fine
> + schemaname name
> + Attribute (column) number in the relation
> + relname name
> +
> + compressmethod char
> +
> +
> + Compression method of the attribute (empty means default)
One thing to keep in mind is that the current compression method is only used
for *new* data - old data can still use the old compression method. It
probably doesn't need to be said here, but maybe you can refer to the docs
about that in alter_table.
> + Number of times the compression was successful (gained a size
> reduction)
It's more clear to say "was reduced in size"
> + /* we assume this inits to all zeroes: */
> + static const PgStat_ToastCounts all_zeroes;
You don't have to assume; static/global allocations are always zero unless
otherwise specified.
Re: pg_receivewal fail to streams when the partial file to write is not fully initialized present in the wal receiver directory
Thanks Michael!
On Sun, Jan 2, 2022 at 11:56 PM Michael Paquier wrote:
> On Sun, Jan 02, 2022 at 09:27:43PM -0800, SATYANARAYANA NARLAPURAM wrote:
> > I noticed that pg_receivewal fails to stream when the partial file to
> write
> > is not fully initialized and fails with the error message something like
> > below. This requires an extra step of deleting the partial file that is
> not
> > fully initialized before starting the pg_receivewal. Attaching a simple
> > patch that creates a temp file, fully initialize it and rename the file
> to
> > the desired wal segment name.
>
> Are you referring to the pre-padding when creating a new partial
> segment, aka when we write chunks of XLOG_BLCKSZ full of zeros until
> the file is fully created? What kind of error did you see? I guess
> that a write() with ENOSPC would be more likely, but you got a
> different problem?
I see two cases, 1/ when no space is left on the device and 2/ when the
process is taken down forcibly (a VM/container crash)
> I don't disagree with improving such cases, but we
> should not do things so as there is a risk of leaving behind an
> infinite set of segments in case of repeated errors
Do you see a problem with the proposed patch that leaves the files behind,
at least in my testing I don't see any files left behind?
> , and partial
> segments are already a kind of temporary file.
>
if the .partial file exists with not zero-padded up to the wal segment size
(WalSegSz), then open_walfile fails with the below error. I have two
options here, 1/ to continue padding the existing partial file and let it
zero up to WalSegSz , 2/create a temp file as I did in the patch. I thought
the latter is safe because it can handle corrupt cases as described below.
Thoughts?
* When streaming to files, if an existing file exists we verify that it's
* either empty (just created), or a complete WalSegSz segment (in which
* case it has been created and padded). Anything else indicates a corrupt
* file. Compressed files have no need for padding, so just ignore this
* case.
>
> - if (dir_data->sync)
> + if (shouldcreatetempfile)
> + {
> + if (durable_rename(tmpsuffixpath, targetpath) != 0)
> + {
> + close(fd);
> + unlink(tmpsuffixpath);
> + return NULL;
> + }
> + }
>
> durable_rename() does a set of fsync()'s, but --no-sync should not
> flush any data.
>
I need to look into this further, without this I am seeing random file
close and rename failures and disconnecting the stream. Also it appears we
are calling durable_rename when we are closing the file (dir_close) even
without --no-sync. Should we worry about the padding case?
> --
> Michael
>
Re: [PATCH] pg_stat_toast v6
On Mon, Jan 03, 2022 at 08:40:50PM +0100, Gunnar "Nick" Bluth wrote: > I wonder why "track_toast.sql" test fails on Windows (with "ERROR: > compression method lz4 not supported"), but "compression.sql" doesn't. > Any hints? The windows CI doesn't have LZ4, so the SQL command fails, but there's an "alternate" expected/compression_1.out so that's accepted. (The regression tests exercise many commands which fail, as expected, like creating an index on an index). If you're going have an alternate file for the --without-lz4 case, then I think you should put it into compression.sql. (But not if you needed an alternate for something else, since we'd need 4 alternates, which is halfway to 8...). -- Justin
Re: TYPCATEGORY_{NETWORK,USER} [was Dubious usage of TYPCATEGORY_STRING]
On 01/03/22 13:55, Tom Lane wrote: > I do see an argument against reclassifying macaddr[8] into > TYPCATEGORY_NETWORK now: we generally expect that if a > category has a preferred type, any member type of the category > can be cast to that preferred type. I was wondering about the details of how that information gets used. It seems partly redundant with what you learn from pg_cast. The CREATE TYPE documentation says: The category and preferred parameters can be used to help control which implicit cast will be applied in ambiguous situations. ... For types that have no implicit casts to or from any other types, it is sufficient to leave these settings at the defaults. However, for a group of related types that have implicit casts, it is often helpful ... which would suggest (to me on a first reading, anyway) that one starts in pg_cast to find out what implicit casts, if any, exist, and then looks to category and preferred if needed to resolve any ambiguity that remains. If understood that way, it doesn't seem to imply any ill effect of having types within a category that might be partitioned into a few disjoint subsets by "implicit cast exists between". (Such subsets might be regarded as autodiscovered mini-categories.) But I could be off-base to understand it that way. Are there spots in the code where the expectation "if a category has a preferred type, any member type of the category can be cast to that preferred type" really takes that stronger form? Hmm, I guess I can see some spots in Chapter 10, in the rules for finding best-match operators or functions, or resolving UNION/CASE types. The UNION/CASE rules look like the effect might be benign: you have step 4, inputs not of the same category => fail, then step 5, where discovery of a preferred type can foreclose consideration of other inputs, then step 6, implicit cast doesn't exist => fail. At first blush, maybe that only fails the same cases that (if you treated implicit-cast-related subsets within a category as mini-categories) you would have failed in step 4. The operator and function resolution rules seem harder to reason about, and yeah, I haven't convinced myself their "any candidate accepts a preferred type => discard candidates accepting non-preferred types" rules couldn't end up discarding the part of the solution space where the solution is, if disjoint "mini-categories" exist. Regards, -Chap
Re: [PATCH] pg_stat_toast v6
Am 03.01.22 um 20:11 schrieb Alvaro Herrera: On 2022-Jan-03, Gunnar "Nick" Bluth wrote: 9:38 $ git format-patch PGDG/master -v5 -o .. ../v5-0001-ping-pong-of-thougths.patch ../v5-0002-ping-pong-of-thougths.patch ../v5-0003-adds-some-debugging-messages-in-toast_helper.c.patch ... Hmm, in such cases I would suggest to create a separate branch and then "git merge --squash" for submission. You can keep your development branch separate, with other merges if you want. I've found this to be easier to manage, though I don't always follow that workflow myself. Using --stdout does help ;-) I wonder why "track_toast.sql" test fails on Windows (with "ERROR: compression method lz4 not supported"), but "compression.sql" doesn't. Any hints? Anyway, I shamelessly copied "wait_for_stats()" from the "stats.sql" file and the tests _should_ now work at least on the platforms with lz4. v6 attached! -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - CatoFrom e743587fbd8f6592bbfa15f53733f79c405000e2 Mon Sep 17 00:00:00 2001 From: "Gunnar \"Nick\" Bluth" Date: Mon, 3 Jan 2022 20:35:05 +0100 Subject: [PATCH v6] pg_stat_toast v6 --- doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/monitoring.sgml | 163 + doc/src/sgml/storage.sgml | 12 +- src/backend/access/table/toast_helper.c | 24 ++ src/backend/catalog/system_views.sql | 20 ++ src/backend/postmaster/pgstat.c | 309 +- src/backend/utils/adt/pgstatfuncs.c | 72 src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/catalog/pg_proc.dat | 25 ++ src/include/pgstat.h | 108 ++ src/test/regress/expected/rules.out | 17 + src/test/regress/expected/track_toast.out | 102 ++ src/test/regress/parallel_schedule| 2 +- src/test/regress/sql/track_toast.sql | 64 15 files changed, 946 insertions(+), 8 deletions(-) create mode 100644 src/test/regress/expected/track_toast.out create mode 100644 src/test/regress/sql/track_toast.sql diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..fa40befc16 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7668,6 +7668,32 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + track_toast (boolean) + + track_toast configuration parameter + + + + +Enables tracking of TOAST activities. +Compressions and externalizations are tracked. +The default is off. +Only superusers can change this setting. + + + + +Be aware that this feature, depending on the amount of TOASTable columns in +your databases, may significantly increase the size of the statistics files +and the workload of the statistics collector. It is recommended to only +temporarily activate this to assess the right compression and storage method +for (a) column(s). + + + + + stats_temp_directory (string) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 62f2a3332b..32d7818096 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -610,6 +610,17 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser yet included in pg_stat_user_functions). + + pg_stat_toastpg_stat_toast + + One row for each column that has ever been TOASTed (compressed and/or externalized). + Showing the number of externalizations, compression attempts / successes, compressed and + uncompressed sizes etc. + + pg_stat_toast for details. + + + pg_stat_slrupg_stat_slru One row per SLRU, showing statistics of operations. See @@ -4969,6 +4980,158 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i + + pg_stat_toast + + + pg_stat_toast + + + + The pg_stat_toast view will contain + one row for each column of variable size that has been TOASTed since + the last statistics reset. The parameter + controls whether TOAST activities are tracked or not. + + + + pg_stat_toast View + + + + + Column Type + + + Description + + + + + + + + schemaname name + + + Name of the schema the relation is in + + + + + + reloid oid + + + OID of the relation + + + + + + attnum int + + +
Re: daitch_mokotoff module
Hi, On 2022-01-02 21:41:53 -0500, Tom Lane wrote: > ... so, that test case is guaranteed to fail in non-UTF8 encodings, > I suppose? I wonder what the LANG environment is in that cfbot > instance. LANG="en_US.UTF-8" But it looks to me like the problem is in the commit cfbot creates, rather than the test run itself: https://github.com/postgresql-cfbot/postgresql/commit/d5b4ec87cfd65dc08d26e1b789bd254405c90a66#diff-388d4bb360a3b24c425e29a85899315dc02f9c1dd9b9bc9aaa828876bdfea50aR56 Greetings, Andres Freund
Re: [PATCH] pg_stat_toast v0.4
On 2022-Jan-03, Gunnar "Nick" Bluth wrote: > 9:38 $ git format-patch PGDG/master -v5 -o .. > ../v5-0001-ping-pong-of-thougths.patch > ../v5-0002-ping-pong-of-thougths.patch > ../v5-0003-adds-some-debugging-messages-in-toast_helper.c.patch > ... Hmm, in such cases I would suggest to create a separate branch and then "git merge --squash" for submission. You can keep your development branch separate, with other merges if you want. I've found this to be easier to manage, though I don't always follow that workflow myself. -- Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/ "Investigación es lo que hago cuando no sé lo que estoy haciendo" (Wernher von Braun)
Re: PostgreSQL stops when adding a breakpoint in CLion
Tomas Vondra writes: > On 1/3/22 16:54, Stanislav Bashkyrtsev wrote: >> - If I put a breakpoint before I start the process then everything works >> fine >> - But if I put/remove a breakpoint after it's fully initialized - the >> process just stops > Why do you think postgres quits? AFAIK CLion uses gdb or lldb for > debugging, which are the debugger of choice for many (most?) hackers on > this list. So that should work fine. FWIW, it's normal in gdb that if you attach to an existing process, the process stops until you say "continue". I know nothing of CLion, but it likely follows that convention too. regards, tom lane
Re: Remove inconsistent quotes from date_part error
Nikhil Benesch writes: > On Mon, Jan 3, 2022 at 1:14 PM Tom Lane wrote: >> Attached v3 restores that distinction, and makes some other small >> tweaks. (I found that there were actually a couple of spots in >> date.c that got it backwards, so admittedly this is a fine point >> that not everybody is on board with. But let's make it consistent >> now.) > LGTM too, for whatever that's worth. OK, pushed. regards, tom lane
Re: TYPCATEGORY_{NETWORK,USER} [was Dubious usage of TYPCATEGORY_STRING]
Chapman Flack writes: > On the same general topic, was there a deliberate choice to put > inet and cidr in TYPCATEGORY_NETWORK but macaddr and macaddr8 > in TYPCATEGORY_USER? Hard to say how "deliberate" it was, at this remove of time. I do see an argument against reclassifying macaddr[8] into TYPCATEGORY_NETWORK now: we generally expect that if a category has a preferred type, any member type of the category can be cast to that preferred type. (The fact that OID is marked preferred breaks that rule, but it holds pretty well otherwise.) I think this is why type interval has its own category rather than being within TYPCATEGORY_DATETIME. regards, tom lane
Re: Throttling WAL inserts when the standby falls behind more than the configured replica_lag_in_bytes
On Wed, Dec 22, 2021 at 4:23 PM SATYANARAYANA NARLAPURAM < satyanarlapu...@gmail.com> wrote: > Hi Hackers, > > I am considering implementing RPO (recovery point objective) enforcement > feature for Postgres where the WAL writes on the primary are stalled when > the WAL distance between the primary and standby exceeds the configured > (replica_lag_in_bytes) threshold. This feature is useful particularly in > the disaster recovery setups where primary and standby are in different > regions and synchronous replication can't be set up for latency and > performance reasons yet requires some level of RPO enforcement. > > The idea here is to calculate the lag between the primary and the standby > (Async?) server during XLogInsert and block the caller until the lag is > less than the threshold value. We can calculate the max lag by iterating > over ReplicationSlotCtl->replication_slots. If this is not something we > don't want to do in the core, at least adding a hook for XlogInsert is of > great value. > > A few other scenarios I can think of with the hook are: > >1. Enforcing RPO as described above >2. Enforcing rate limit and slow throttling when sync standby is >falling behind (could be flush lag or replay lag) >3. Transactional log rate governance - useful for cloud providers to >provide SKU sizes based on allowed WAL writes. > > Thoughts? > Very similar requirement or need was discussed in the past in [1], not exactly RPO enforcement but large bulk operation/transaction negatively impacting concurrent transactions due to replication lag. Would be good to refer to that thread as it explains the challenges for implementing functionality mentioned in this thread. Mostly the challenge being no common place to code the throttling logic instead requiring calls to be sprinkled around in various parts. 1] https://www.postgresql.org/message-id/flat/CA%2BU5nMLfxBgHQ1VLSeBHYEMjHXz_OHSkuFdU6_1quiGM0RNKEg%40mail.gmail.com
Re: Remove inconsistent quotes from date_part error
On Mon, Jan 3, 2022 at 1:14 PM Tom Lane wrote: > Hmm, I think you went a bit too far here. The existing code intends > to draw a distinction between "not recognized" (i.e., "we don't know > what that word was you used") and "not supported" (i.e., "we know > that word, but it doesn't seem to make sense in context, or we > haven't got round to the case yet"). You've mashed those into the > same error text, which I don't think we should do, especially > since we're using distinct ERRCODE values for them. Oops. I noticed that "inconsistency" between ERRCODE_FEATURE_NOT_SUPPORTED and ERRCODE_INVALID_PARAMETER_VALUE and then promptly blazed past it. Thanks for catching that. > Attached v3 restores that distinction, and makes some other small > tweaks. (I found that there were actually a couple of spots in > date.c that got it backwards, so admittedly this is a fine point > that not everybody is on board with. But let's make it consistent > now.) LGTM too, for whatever that's worth.
Re: PostgreSQL stops when adding a breakpoint in CLion
On 1/3/22 16:54, Stanislav Bashkyrtsev wrote: I tried debugging PostgreSQL to better understand how it works. It worked fine a day ago, but for some reason I have issues with debugging now: - If I put a breakpoint before I start the process then everything works fine - But if I put/remove a breakpoint after it's fully initialized - the process just stops And when reading the next command postgres.c, I see that input_message is empty. I assume CLion sends a signal which awakens PostgreSQL, but there's no data on the input? But should PostgreSQL quit in such a situation? Why do you think postgres quits? AFAIK CLion uses gdb or lldb for debugging, which are the debugger of choice for many (most?) hackers on this list. So that should work fine. The way I build and start: make clean ./configure --enable-cassert --enable-debug CFLAGS="-ggdb -O0 -g3 -fno-omit-frame-pointer" make make install /usr/local/pgsql/bin/initdb -D /Users/stas/projects/postgres/data Starting command: /usr/local/pgsql/bin/postgres --single -D /Users/stas/projects/postgres/data postgres Now sure why you start it in single-user mode, but I don't think that should affect debugging. Try redirecting the output to a log file, maybe that'll tell you what happened. regards -- Tomas Vondra EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company
Re: [PATCH] pg_stat_toast v0.4
Am 03.01.22 um 19:30 schrieb Alvaro Herrera: On 2022-Jan-03, Gunnar "Nick" Bluth wrote: Am 03.01.22 um 17:50 schrieb Justin Pryzby: Soon you will think this is fun :) As long as you're happy with plain patches like the attached one, I may ;-) Well, with a zero-byte patch, not very much ... D'oh BTW why do you number with a "0." prefix? It could just be "4" and "5" and so on. There's no value in two-part version numbers for patches. Also, may I suggest that "git format-patch -vN" with varying N is an easier way to generate patches to attach? Not when you have a metric ton of commits in the history... I'll hopefully find a way to start over soon :/ 9:38 $ git format-patch PGDG/master -v5 -o .. ../v5-0001-ping-pong-of-thougths.patch ../v5-0002-ping-pong-of-thougths.patch ../v5-0003-adds-some-debugging-messages-in-toast_helper.c.patch ../v5-0004-adds-some-groundwork-for-pg_stat_toast-to-pgstat..patch ../v5-0005-fixes-wrong-type-for-pgstat_track_toast-GUC.patch ../v5-0006-introduces-PgStat_BackendAttrIdentifier-OID-attr-.patch ../v5-0007-implements-and-calls-pgstat_report_toast_activity.patch ../v5-0008-Revert-adds-some-debugging-messages-in-toast_help.patch ../v5-0009-adds-more-detail-to-logging.patch ../v5-0010-adds-toastactivity-to-table-stats-and-many-helper.patch ../v5-0011-fixes-missed-replacement-in-comment.patch ../v5-0012-adds-SQL-support-functions.patch ../v5-0013-Add-SQL-functions.patch ../v5-0014-reset-to-HEAD.patch ../v5-0015-makes-DEBUG2-messages-more-precise.patch ../v5-0016-adds-timing-information-to-stats-and-view.patch ../v5-0017-adds-a-basic-set-of-tests.patch ../v5-0018-adds-a-basic-set-of-tests.patch ../v5-0019-chooses-a-PGSTAT_TOAST_HASH_SIZE-of-64-changes-ha.patch ../v5-0020-removes-whitespace-trash.patch ../v5-0021-returns-to-PGDG-master-.gitignore.patch ../v5-0022-pg_stat_toast_v0.5.patch ../v5-0023-moves-tests-to-regress.patch But alas! INT versioning it is from now on! -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - Catodiff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..fa40befc16 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7668,6 +7668,32 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + track_toast (boolean) + + track_toast configuration parameter + + + + +Enables tracking of TOAST activities. +Compressions and externalizations are tracked. +The default is off. +Only superusers can change this setting. + + + + +Be aware that this feature, depending on the amount of TOASTable columns in +your databases, may significantly increase the size of the statistics files +and the workload of the statistics collector. It is recommended to only +temporarily activate this to assess the right compression and storage method +for (a) column(s). + + + + + stats_temp_directory (string) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 62f2a3332b..32d7818096 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -610,6 +610,17 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser yet included in pg_stat_user_functions). + + pg_stat_toastpg_stat_toast + + One row for each column that has ever been TOASTed (compressed and/or externalized). + Showing the number of externalizations, compression attempts / successes, compressed and + uncompressed sizes etc. + + pg_stat_toast for details. + + + pg_stat_slrupg_stat_slru One row per SLRU, showing statistics of operations. See @@ -4969,6 +4980,158 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i + + pg_stat_toast + + + pg_stat_toast + + + + The pg_stat_toast view will contain + one row for each column of variable size that has been TOASTed since + the last statistics reset. The parameter + controls whether TOAST activities are tracked or not. + + + + pg_stat_toast View + + + + + Column Type + + + Description + + + + + + + + schemaname name + + + Name of the schema the relation is in + + + + + + reloid oid + + + OID of the relation + + + + + + attnum int + + + Attribute (column) number in the relation + + + + + + relname name + + + Name of the relation + + + + + + attname
TYPCATEGORY_{NETWORK,USER} [was Dubious usage of TYPCATEGORY_STRING]
On 12/02/21 16:22, Tom Lane wrote: > taking the special types out of the string category, so 0002 > attached invents a separate TYPCATEGORY_INTERNAL category to > put them in. On the same general topic, was there a deliberate choice to put inet and cidr in TYPCATEGORY_NETWORK but macaddr and macaddr8 in TYPCATEGORY_USER? It looks like macaddr was put in category U (macaddr8 didn't exist yet) in bac3e83, the same commit that put inet and cidr into category I, apparently in order to "hew exactly to the behavior of the previous hardwired logic", on the principle that "any adjustment of the standard set of categories should be done separately". The birth of macaddr looks to have been back in 1998 in 2d69fd9, the same commit that added 'ipaddr'. Neither was added at that time to the hardcoded switch in TypeCategory(). The plot thickens ipaddr became inet in 8849655 (8 Oct 1998). cidr was added in 858a3b5 (21 Oct 1998). Then ca2995 added NETWORK_TYPE to TypeCategory and put inet and cidr in it (22 Oct 1998). Looks like that was done to reduce duplication of pg_proc entries between inet and cidr by allowing implicit coercion. And I guess you wouldn't want to suggest the existence of coercions between MAC addresses and inet addresses. But there aren't any such casts present in pg_cast anyway, so is that a persuasive present-day rationale for the (otherwise odd-seeming) split of these types across categories? They are grouped in a single documentation "category". Regards, -Chap
Re: Remove inconsistent quotes from date_part error
On 2022-Jan-03, Tom Lane wrote:
> Attached v3 restores that distinction, and makes some other small
> tweaks. (I found that there were actually a couple of spots in
> date.c that got it backwards, so admittedly this is a fine point
> that not everybody is on board with. But let's make it consistent
> now.)
LGTM.
> @@ -2202,9 +2204,9 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
> case DTK_ISOYEAR:
> default:
> ereport(ERROR,
> -
> (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
> - errmsg("\"time\" units \"%s\"
> not recognized",
> - lowunits)));
> +
> (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
> + errmsg("unit \"%s\" not
> supported for type %s",
> + lowunits,
> format_type_be(TIMEOID;
I agree that these changes are an improvement.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Introducing PgVA aka PostgresVectorAcceleration using SIMD vector instructions starting with hex_encode
On Fri, Dec 31, 2021 at 9:32 AM Hans Buschmann wrote: > Inspired by the effort to integrate JIT for executor acceleration I thought > selected simple algorithms working with array-oriented data should be > drastically accelerated by using SIMD instructions on modern hardware. Hi Hans, I have experimented with SIMD within Postgres last year, so I have some idea of the benefits and difficulties. I do think we can profit from SIMD more, but we must be very careful to manage complexity and maximize usefulness. Hopefully I can offer some advice. > - restrict on 64 -bit architectures > These are the dominant server architectures, have the necessary data > formats and corresponding registers and operating instructions > - start with Intel x86-64 SIMD instructions: > This is the vastly most used platform, available for development and > in practical use > - don’t restrict the concept to only Intel x86-64, so that later people with > more experience on other architectures can jump in and implement comparable > algorithms > - fallback to the established implementation in postgres in non appropriate > cases or on user request (GUC) These are all reasonable goals, except GUCs are the wrong place to choose hardware implementations -- it should Just Work. > - coding for maximum hardware usage instead of elegant programming > Once tested, the simple algorithm works as advertised and is used to > replace most execution parts of the standard implementaion in C -1 Maintaining good programming style is a key goal of the project. There are certainly non-elegant parts in the code, but that has a cost and we must consider tradeoffs carefully. I have read some of the optimized code in glibc and it is not fun. They at least know they are targeting one OS and one compiler -- we don't have that luxury. > - focus optimization for the most advanced SIMD instruction set: AVX512 > This provides the most advanced instructions and quite a lot of > large registers to aid in latency avoiding -1 AVX512 is a hodge-podge of different instruction subsets and are entirely lacking on some recent Intel server hardware. Also only available from a single chipmaker thus far. > - The loops implementing the algorithm are written in NASM assembler: > NASM is actively maintained, has many output formats, follows the > Intel style, has all current instrucions implemented and is fast. > - The loops are mostly independent of operating systems, so all OS’s basing > on a NASM obj output format are supported: > This includes Linux and Windows as the most important ones > - The algorithms use advanced techniques (constant and temporary registers) > to avoid most unnessary memory accesses: > The assembly implementation gives you the full control over the > registers (unlike intrinsics) On the other hand, intrinsics are easy to integrate into a C codebase and relieve us from thinking about object formats. A performance feature that happens to work only on common OS's is probably fine from the user point of view, but if we have to add a lot of extra stuff to make it work at all, that's not a good trade off. "Mostly independent" of the OS is not acceptable -- we shouldn't have to think about the OS at all when the coding does not involve OS facilities (I/O, processes, etc). > As an example I think of pg_dump to dump a huge amount of bytea data (not > uncommon in real applications). Most of these data are in toast tables, often > uncompressed due to their inherant structure. The dump must read the toast > pages into memory, decompose the page, hexdump the content, put the result in > an output buffer and trigger the I/O. By integrating all these steps into one > big performance improvements can be achieved (but naturally not here in my > first implementation!). Seems like a reasonable area to work on, but I've never measured. > The best result I could achieve was roughly 95 seconds for 1 Million dumps of > 1718 KB on a Tigerlake laptop using AVX512. This gives about 18 GB/s > source-hexdumping rate on a single core! > > In another run with postgres the time to hexdump about half a million tuples > with a bytea column yeilding about 6 GB of output reduced the time from about > 68 seconds to 60 seconds, which clearly shows the postgres overhead for > executing the copy command on such a data set. I don't quite follow -- is this patched vs. unpatched Postgres? I'm not sure what's been demonstrated. > The assembler routines should work on most x86-64 operating systems, but for > the moment only elf64 and WIN64 output formats are supported. > > The standard calling convention is followed mostly in the LINUX style, on > Windows the parameters are moved around accordingly. The same > assembler-source-code can be used on both platforms. > I have updated the makefile to include the nasm command and the nasm flags, > but I need help to make these based on configure. >
Re: [PATCH] pg_stat_toast v0.4
On 2022-Jan-03, Gunnar "Nick" Bluth wrote: > Am 03.01.22 um 17:50 schrieb Justin Pryzby: > > Soon you will think this is fun :) > > As long as you're happy with plain patches like the attached one, I may ;-) Well, with a zero-byte patch, not very much ... BTW why do you number with a "0." prefix? It could just be "4" and "5" and so on. There's no value in two-part version numbers for patches. Also, may I suggest that "git format-patch -vN" with varying N is an easier way to generate patches to attach? -- Álvaro Herrera Valdivia, Chile — https://www.EnterpriseDB.com/ "Hay quien adquiere la mala costumbre de ser infeliz" (M. A. Evans)
Re: [PoC] Delegating pg_ident to a third party
On Mon, 2022-01-03 at 12:36 -0500, Stephen Frost wrote: > * Jacob Champion (pchamp...@vmware.com) wrote: > > On Fri, 2021-12-17 at 10:06 +0100, Peter Eisentraut wrote: > > > On 17.12.21 00:48, Jacob Champion wrote: > > > > WDYT? (My responses here will be slower than usual. Hope you all have a > > > > great end to the year!) > > > > > > Looks interesting. I wonder whether putting this into pg_ident.conf is > > > sensible. I suspect people will want to eventually add more features > > > around this, like automatically creating roles or role memberships, at > > > which point pg_ident.conf doesn't seem appropriate anymore. > > This is the part that I really wonder about also ... I've always viewed > pg_ident as being intended mainly for one-to-one kind of mappings and > not the "map a bunch of different users into the same role" that this > advocated for. Being able to have roles and memberships automatically > created is much more the direction that I'd say we should be going in, > so that in-database auditing has an actual user to go on and not some > generic role that could be any number of people. That last point was my motivation for the authn_id patch [1] -- so that auditing could see the actual user _and_ the generic role. The information is already there to be used, it's just not exposed to the stats framework yet. Forcing one role per individual end user is wasteful and isn't really making good use of the role-based system that you already have. Generally speaking, when administering hundreds or thousands of users, people start dividing them up into groups as opposed to dealing with them individually. So I don't think new features should be taking away flexibility in this area -- if one role per user already works well for you, great, but don't make everyone do the same. > I'd go a step further and suggest that the way to do this is with a > background worker that's started up and connects to an LDAP > infrastructure and listens for changes, allowing the system to pick up > on new roles/memberships as soon as they're created in the LDAP > environment. That would then be controlled by appropriate settings in > postgresql.conf/.auto.conf. This is roughly what you can already do with existing (third-party) tools, and that approach isn't scaling out in practice for some of our existing customers. The load on the central server, for thousands of idle databases dialing in just to see if there are any new users, is huge. > All that said, I do see how having the ability to call out to another > system for mappings may be useful, so I'm not sure that we shouldn't > consider this specific change and have it be specifically just for > mappings, in which case pg_ident seems appropriate. Yeah, this PoC was mostly an increment on the functionality that already existed. The division between what goes in pg_hba and what goes in pg_ident is starting to blur with this patchset, though, and I think Peter's point is sound. > I certainly don't think we should have this be limited to LDAP auth- > such an external mapping ability is suitable for any authentication > method that supports a mapping (thinking specifically of GSSAPI, of > course..). Not sure if that's what was meant above but did want to > make sure that was clear. You can't use usermaps with LDAP auth yet, so no, that's not what I meant. (I have another patch for that feature in commitfest, which would allow these two things to be used together.) Thanks, --Jacob [1] https://www.postgresql.org/message-id/flat/E1lTwp4-0002l4-L9%40gemulon.postgresql.org
PostgreSQL stops when adding a breakpoint in CLion
I tried debugging PostgreSQL to better understand how it works. It worked fine a day ago, but for some reason I have issues with debugging now: - If I put a breakpoint before I start the process then everything works fine - But if I put/remove a breakpoint after it's fully initialized - the process just stops And when reading the next command postgres.c, I see that input_message is empty. I assume CLion sends a signal which awakens PostgreSQL, but there's no data on the input? But should PostgreSQL quit in such a situation? The way I build and start: make clean ./configure --enable-cassert --enable-debug CFLAGS="-ggdb -O0 -g3 -fno-omit-frame-pointer" make make install /usr/local/pgsql/bin/initdb -D /Users/stas/projects/postgres/data Starting command: /usr/local/pgsql/bin/postgres --single -D /Users/stas/projects/postgres/data postgres
Re: Add spin_delay() implementation for Arm in s_lock.h
Tom, Hope everything is well going into the new year. I'd like to pick this discussion back up and your thoughts on the patch with the data I posted 2 weeks prior. Is there more data that would be helpful? Different setup? Data on older versions of Postgresql to ascertain if it makes more sense on versions before the large re-work of the snapshot algorithm that exhibited quite a bit of synchronization contention? Thanks, Geoff
Re: Remove inconsistent quotes from date_part error
Nikhil Benesch writes:
> Updated patch attached.
Hmm, I think you went a bit too far here. The existing code intends
to draw a distinction between "not recognized" (i.e., "we don't know
what that word was you used") and "not supported" (i.e., "we know
that word, but it doesn't seem to make sense in context, or we
haven't got round to the case yet"). You've mashed those into the
same error text, which I don't think we should do, especially
since we're using distinct ERRCODE values for them.
Attached v3 restores that distinction, and makes some other small
tweaks. (I found that there were actually a couple of spots in
date.c that got it backwards, so admittedly this is a fine point
that not everybody is on board with. But let's make it consistent
now.)
regards, tom lane
diff --git a/src/backend/utils/adt/date.c b/src/backend/utils/adt/date.c
index 9e8baeaa9c..629166632d 100644
--- a/src/backend/utils/adt/date.c
+++ b/src/backend/utils/adt/date.c
@@ -22,6 +22,7 @@
#include
#include "access/xact.h"
+#include "catalog/pg_type.h"
#include "common/hashfn.h"
#include "libpq/pqformat.h"
#include "miscadmin.h"
@@ -1124,8 +1125,8 @@ extract_date(PG_FUNCTION_ARGS)
default:
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("date units \"%s\" not supported",
-lowunits)));
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(DATEOID;
}
}
else if (type == UNITS)
@@ -1207,8 +1208,8 @@ extract_date(PG_FUNCTION_ARGS)
default:
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("date units \"%s\" not supported",
-lowunits)));
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(DATEOID;
intresult = 0;
}
}
@@ -1223,8 +1224,8 @@ extract_date(PG_FUNCTION_ARGS)
default:
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("date units \"%s\" not supported",
-lowunits)));
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(DATEOID;
intresult = 0;
}
}
@@ -1232,7 +1233,8 @@ extract_date(PG_FUNCTION_ARGS)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("date units \"%s\" not recognized", lowunits)));
+ errmsg("unit \"%s\" not recognized for type %s",
+ lowunits, format_type_be(DATEOID;
intresult = 0;
}
@@ -2202,9 +2204,9 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
case DTK_ISOYEAR:
default:
ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"time\" units \"%s\" not recognized",
-lowunits)));
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(TIMEOID;
intresult = 0;
}
}
@@ -2219,8 +2221,8 @@ time_part_common(PG_FUNCTION_ARGS, bool retnumeric)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"time\" units \"%s\" not recognized",
- lowunits)));
+ errmsg("unit \"%s\" not recognized for type %s",
+ lowunits, format_type_be(TIMEOID;
intresult = 0;
}
@@ -2980,9 +2982,9 @@ timetz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
case DTK_MILLENNIUM:
default:
ereport(ERROR,
- (errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"time with time zone\" units \"%s\" not recognized",
-lowunits)));
+ (errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(TIMETZOID;
intresult = 0;
}
}
@@ -3001,8 +3003,8 @@ timetz_part_common(PG_FUNCTION_ARGS, bool retnumeric)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("\"time with time zone\" units \"%s\" not recognized",
- lowunits)));
+ errmsg("unit \"%s\" not recognized for type %s",
+ lowunits, format_type_be(TIMETZOID;
intresult = 0;
}
diff --git a/src/backend/utils/adt/timestamp.c b/src/backend/utils/adt/timestamp.c
index cb9faff0bb..b66d69db00 100644
--- a/src/backend/utils/adt/timestamp.c
+++ b/src/backend/utils/adt/timestamp.c
@@ -3972,8 +3972,8 @@ timestamp_trunc(PG_FUNCTION_ARGS)
default:
ereport(ERROR,
(errcode(ERRCODE_FEATURE_NOT_SUPPORTED),
- errmsg("timestamp units \"%s\" not supported",
-lowunits)));
+ errmsg("unit \"%s\" not supported for type %s",
+lowunits, format_type_be(TIMESTAMPOID;
result = 0;
}
@@ -3986,8 +3986,8 @@ timestamp_trunc(PG_FUNCTION_ARGS)
{
ereport(ERROR,
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
- errmsg("timestamp units \"%s\" not recognized",
- lowunits)));
+ errmsg("unit \"%s\" not recognized for type %s",
+ lowunits, format_type_be(TIMESTAMPOID;
result = 0;
}
@@ -4165,8 +4165,8 @@
Re: [PATCH] pg_stat_toast v0.4
Am 03.01.22 um 17:50 schrieb Justin Pryzby: On Mon, Jan 03, 2022 at 05:00:45PM +0100, Gunnar "Nick" Bluth wrote: Am 03.01.22 um 16:52 schrieb Gunnar "Nick" Bluth: pg_stat_toast_v0.4.patch attached. Note that the cfbot says this fails under windows Thanks for the heads up! http://cfbot.cputube.org/gunnar-quotnickquot-bluth.html ... [16:47:05.347] Could not determine contrib module type for track_toast [16:47:05.347] at src/tools/msvc/mkvcbuild.pl line 31. Not only Window$... as it turns out, one of the checks was pretty bogus. Kicked that one and instead wrote two (hopefully) meaningful ones. Also, I moved the tests to regress/, as they're not really for a module anyway. Let's see how this fares! Aaaand I attached a former version of the patch file... sorry, I'm kind of struggling with all the squashing/rebasing... Soon you will think this is fun :) As long as you're happy with plain patches like the attached one, I may ;-) All the best, -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - Cato
Re: [PoC] Delegating pg_ident to a third party
Greetings, * Jacob Champion (pchamp...@vmware.com) wrote: > On Fri, 2021-12-17 at 10:06 +0100, Peter Eisentraut wrote: > > On 17.12.21 00:48, Jacob Champion wrote: > > > WDYT? (My responses here will be slower than usual. Hope you all have a > > > great end to the year!) > > > > Looks interesting. I wonder whether putting this into pg_ident.conf is > > sensible. I suspect people will want to eventually add more features > > around this, like automatically creating roles or role memberships, at > > which point pg_ident.conf doesn't seem appropriate anymore. This is the part that I really wonder about also ... I've always viewed pg_ident as being intended mainly for one-to-one kind of mappings and not the "map a bunch of different users into the same role" that this advocated for. Being able to have roles and memberships automatically created is much more the direction that I'd say we should be going in, so that in-database auditing has an actual user to go on and not some generic role that could be any number of people. I'd go a step further and suggest that the way to do this is with a background worker that's started up and connects to an LDAP infrastructure and listens for changes, allowing the system to pick up on new roles/memberships as soon as they're created in the LDAP environment. That would then be controlled by appropriate settings in postgresql.conf/.auto.conf. > Yeah, pg_ident is getting too cramped for this. All that said, I do see how having the ability to call out to another system for mappings may be useful, so I'm not sure that we shouldn't consider this specific change and have it be specifically just for mappings, in which case pg_ident seems appropriate. > > Should we have a new file for this? Do you have any further ideas? > > My experience with these configs is mostly limited to HTTP servers. > That said, it's pretty hard to beat the flexibility of arbitrary key- > value pairs inside nested contexts. It's nice to be able to say things > like > > Everyone has to use LDAP auth > With this server > And these TLS settings > > Except admins > who additionally need client certificates > with this CA root > > And Jacob > who isn't allowed in anymore I certainly don't think we should have this be limited to LDAP auth- such an external mapping ability is suitable for any authentication method that supports a mapping (thinking specifically of GSSAPI, of course..). Not sure if that's what was meant above but did want to make sure that was clear. The rest looks a lot more like pg_hba or perhaps in-database privileges like roles/memberships existing or not and CONNECT rights. I'm not really sold on the idea of adding yet even more different ways to control authorization. Thanks, Stephen signature.asc Description: PGP signature
Re: Use MaxLockMode in lock methods initialization
Michael Paquier writes: > On Mon, Jan 03, 2022 at 02:47:22PM +0800, Julien Rouhaud wrote: >> While reading some code around I noticed that b04aeb0a053e7 added a >> MaxLockMode >> but didn't update the lock methods initialization. It shouldn't make much >> difference in the long run but some consistency seems better to me. > Makes sense to me. MaxLockMode is here for the same purpose as this > initialization area. Agreed. That aspect of b04aeb0a053e7 was a bit of a quick hack, and it didn't occur to me to look for other places where the symbol could be used. But these two places are spot-on for it. Pushed with a bit of comment-fiddling. regards, tom lane
Re: Proposal: sslmode=tls-only
On Fri, 2021-12-24 at 14:08 +, Keith Burdis wrote: > Has consideration been given to having something like ssl-mode=tls- > only where the SSLRequest message is skipped and the TLS handshake > starts immediately with the protocol continuing after that? From an implementation standpoint, I think I'd prefer to keep sslmode independent from the new implicit-TLS setting, so that any existing deployments can migrate to the new handshake without needing to change their certificate setup. (That said, any sslmodes weaker than `require` would be incompatible with the new setting.) --Jacob
Re: refactoring basebackup.c
On 11/22/21 11:05 PM, Jeevan Ladhe wrote: Please find the lz4 compression patch here that basically has: Please refer to this scenario , where --server-compression is only compressing base backup into lz4 format but not pg_wal directory [edb@centos7tushar bin]$ ./pg_basebackup -Ft --server-compression=lz4 -Xstream -D foo [edb@centos7tushar bin]$ ls foo backup_manifest base.tar.lz4 pg_wal.tar this same is valid for gzip as well if server-compression is set to gzip edb@centos7tushar bin]$ ./pg_basebackup -Ft --server-compression=gzip4 -Xstream -D foo1 [edb@centos7tushar bin]$ ls foo1 backup_manifest base.tar.gz pg_wal.tar if this scenario is valid then both the folders format should be in lz4 format otherwise we should get an error something like - not a valid option ? -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: [PATCH] pg_stat_toast v0.4
On Mon, Jan 03, 2022 at 05:00:45PM +0100, Gunnar "Nick" Bluth wrote: > Am 03.01.22 um 16:52 schrieb Gunnar "Nick" Bluth: > > > pg_stat_toast_v0.4.patch attached. Note that the cfbot says this fails under windows http://cfbot.cputube.org/gunnar-quotnickquot-bluth.html ... [16:47:05.347] Could not determine contrib module type for track_toast [16:47:05.347] at src/tools/msvc/mkvcbuild.pl line 31. > Aaaand I attached a former version of the patch file... sorry, I'm kind of > struggling with all the squashing/rebasing... Soon you will think this is fun :) -- Justin
Re: [PoC] Delegating pg_ident to a third party
On Fri, 2021-12-17 at 10:06 +0100, Peter Eisentraut wrote: > On 17.12.21 00:48, Jacob Champion wrote: > > WDYT? (My responses here will be slower than usual. Hope you all have a > > great end to the year!) > > Looks interesting. I wonder whether putting this into pg_ident.conf is > sensible. I suspect people will want to eventually add more features > around this, like automatically creating roles or role memberships, at > which point pg_ident.conf doesn't seem appropriate anymore. Yeah, pg_ident is getting too cramped for this. > Should we have a new file for this? Do you have any further ideas? My experience with these configs is mostly limited to HTTP servers. That said, it's pretty hard to beat the flexibility of arbitrary key- value pairs inside nested contexts. It's nice to be able to say things like Everyone has to use LDAP auth With this server And these TLS settings Except admins who additionally need client certificates with this CA root And Jacob who isn't allowed in anymore Are there any existing discussions along these lines that I should take a look at? --Jacob
Re: Index-only scans vs. partially-retrievable indexes
> Andrey Borodin writes: > >> I've tried to toy with the patch and remembered one related caveat. >> If we have index for both returnable and nonreturnable attributes, IOS will >> not be choosen: > >> postgres=# create index on t using gist(a gist_trgm_ops) include (a); >> postgres=# explain select * from t where a like 'z'; >> QUERY PLAN >> - >> Index Scan using t_a_a1_idx on t (cost=0.12..8.14 rows=1 width=32) >> Index Cond: (a ~~ 'z'::text) >> (2 rows) > > This case is improved by 0002, no? > Uhmm, yes, you are right. Works as expected with the second patch. I tried first patch against this before writing. But did not expect much from a revert... Thanks you! Best regards, Andrey Borodin.
Re: psql - add SHOW_ALL_RESULTS option
On 23.12.21 12:40, Fabien COELHO wrote: This is also a feature/bug of libpq which happens to be hidden by PQexec: when one command crashes PQgetResult actually returns *2* results. First one with the FATAL message, second one when libpq figures out that the connection was lost with the second message appended to the first. PQexec just happen to silently ignore the first result. I added a manual reset of the error message when first shown so that it is not shown twice. It is unclear to me whether the reset should be somewhere in libpq instead. I added a voluntary crash at the end of the psql test. With this "voluntary crash", the regression test output is now psql ... ok (test process exited with exit code 2) 281 ms Normally, I'd expect this during development if there was a crash somewhere, but showing this during a normal run now, and moreover still saying "ok", is quite weird and confusing. Maybe this type of test should be done in the TAP framework instead.
Re: daitch_mokotoff module
Dag Lem writes: > Tom Lane writes: >> (We do have methods for dealing with non-ASCII test cases, but >> I can't see that this patch is using any of them.) > I naively assumed that tests would be run in an UTF8 environment. Nope, not necessarily. Our current best practice for this is to separate out encoding-dependent test cases into their own test script, and guard the script with an initial test on database encoding. You can see an example in src/test/modules/test_regex/sql/test_regex_utf8.sql and the two associated expected-files. It's a good idea to also cover as much as you can with pure-ASCII test cases that will run regardless of the prevailing encoding. > Running "ack -l '[\x80-\xff]'" in the contrib/ directory reveals that > two other modules are using UTF8 characters in tests - citext and > unaccent. Yeah, neither of those have been upgraded to said best practice. (If you feel like doing the legwork to improve that situation, that'd be great.) > Looking into the unaccent module, I don't quite understand how it will > work with various encodings, since it doesn't seem to decode its input - > will it fail if run under anything but ASCII or UTF8? Its Makefile seems to be forcing the test database to use UTF8. I think this is a less-than-best-practice choice, because then we have zero test coverage for other encodings; but it does prevent test failures. regards, tom lane
Re: Remove extra spaces
Suraj Kharage writes: > While browsing the code, noticed the extra spaces after the function name. > Removed the same in the attached patch. I'm afraid that's a waste of time because the next pgindent run will just put them back. "numeric" is also a typedef name and this usage of it seems to confuse pgindent. If you wanted to dive into the pgindent code and fix that bug in it, that'd be great, but the return-on-effort is probably going to be poor. (Another possibility is to change the C function name.) regards, tom lane
Re: [PATCH] Accept IP addresses in server certificate SANs
On Sun, 2022-01-02 at 13:29 -0800, Andres Freund wrote: > Hi, > > On 2021-12-16 01:13:57 +, Jacob Champion wrote: > > Attached is a patch for libpq to support IP addresses in the server's > > Subject Alternative Names, which would allow admins to issue certs for > > multiple IP addresses, both IPv4 and IPv6, and mix them with > > alternative DNS hostnames. These addresses are compared bytewise > > instead of stringwise, so the client can contact the server via > > alternative spellings of the same IP address. > > This fails to build on windows: > https://nam04.safelinks.protection.outlook.com/?url=https%3A%2F%2Fcirrus-ci.com%2Ftask%2F6734650927218688%3Flogs%3Dbuild%23L1029data=04%7C01%7Cpchampion%40vmware.com%7C2b2171168f3c4935e89f08d9ce36f790%7Cb39138ca3cee4b4aa4d6cd83d9dd62f0%7C0%7C0%7C637767557770534489%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C3000sdata=JtfsPtershSljU1oDGrkL8bQiHYB3iMfUgTqlh%2B4wbs%3Dreserved=0 > > [14:33:28.277] network.obj : error LNK2019: unresolved external symbol > pg_inet_net_pton referenced in function network_in > [c:\cirrus\postgres.vcxproj] Thanks for the heads up; I'll fix that while I'm implementing the internal API. --Jacob
Re: [PATCH] Accept IP addresses in server certificate SANs
On Fri, 2021-12-17 at 16:54 +0900, Kyotaro Horiguchi wrote: > Sorry for the silly mistake. > > At Fri, 17 Dec 2021 15:40:10 +0900 (JST), Kyotaro Horiguchi > wrote in > > > NSS departs slightly from the spec and will additionally try to match > > > an IP address against the CN, but only if there are no iPAddresses in > > > the SAN. It roughly matches the logic for DNS names. > > > > OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN > > doesn't exist. X509_check_ip() tries SAN and completely ignores > > iPAdress and CN. > > OpenSSL seems different. X509_check_host() tries SAN then CN iff SAN > doesn't exist. X509_check_ip() tries iPAddress and completely ignores > CN. Right. On Fri, 2021-12-17 at 15:40 +0900, Kyotaro Horiguchi wrote: > > > +* pg_inet_net_pton() will accept CIDR masks, which we > > don't want to > > > +* match, so skip the comparison if the host string > > > contains a slash. > > > +*/ > > > + if (!strchr(host, '/') > > > + && pg_inet_net_pton(PGSQL_AF_INET6, host, addr, > > > -1) == 128) > > > > > > If a cidr is given, pg_inet_net_pton returns a number less than 128 so > > > we don't need to check '/' explicity? (I'm not sure '/128' is > > > sensible but doesn't harm..) > > > > Personally I think that, if someone wants your libpq to connect to a > > server with a hostname of "some:ipv6::address/128", then they are > > trying to pull something (evading a poorly coded blocklist, perhaps?) > > and we should not allow that to match an IP. Thoughts? > > If the client could connect to the network-address, it could be said > that we can assume that address is the name:p Just kidding. > > As the name suggests, the function reads a network address. And the > only user is network_in(). I think we should provide pg_inet_pton() > instead of abusing pg_inet_net_pton(). inet_net_pton_*() functions > can be modified to reject /cidr part without regression so we are able > to have pg_inet_pton() with a small amount of change. > > - inet_net_pton_ipv4(const char *src, u_char *dst) > + inet_net_pton_ipv4_internal(const char *src, u_char *dst, bool netaddr) > > + inet_net_pton_ipv4(const char *src, u_char *dst) > (calls inet_net_pton_ipv4_internal(src, dst, true)) > + inet_pton_ipv4(const char *src, u_char *dst) > (calls inet_net_pton_ipv4_internal(src, dst, false)) Sounds good, I will make that change. Thanks for the feedback! --Jacob
Re: [PATCH] pg_stat_toast v0.4
Am 03.01.22 um 16:52 schrieb Gunnar "Nick" Bluth: pg_stat_toast_v0.4.patch attached. Aaaand I attached a former version of the patch file... sorry, I'm kind of struggling with all the squashing/rebasing... -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - CatoFrom 560cfb4082804709d952b3f741b505025f400f97 Mon Sep 17 00:00:00 2001 From: "Gunnar \"Nick\" Bluth" Date: Mon, 3 Jan 2022 16:18:36 +0100 Subject: [PATCH] pg_stat_toast_v0.4 --- doc/src/sgml/config.sgml | 26 ++ doc/src/sgml/monitoring.sgml | 163 + doc/src/sgml/storage.sgml | 12 +- src/backend/access/table/toast_helper.c | 24 ++ src/backend/catalog/system_views.sql | 20 ++ src/backend/postmaster/pgstat.c | 309 +- src/backend/utils/adt/pgstatfuncs.c | 72 src/backend/utils/misc/guc.c | 9 + src/backend/utils/misc/postgresql.conf.sample | 1 + src/include/catalog/pg_proc.dat | 25 ++ src/include/pgstat.h | 108 ++ src/test/modules/Makefile | 1 + src/test/modules/track_toast/Makefile | 18 + .../track_toast/expected/track_toast.out | 55 .../modules/track_toast/sql/track_toast.sql | 20 ++ src/test/modules/track_toast/track_toast.conf | 1 + src/test/regress/expected/rules.out | 17 + 17 files changed, 874 insertions(+), 7 deletions(-) create mode 100644 src/test/modules/track_toast/Makefile create mode 100644 src/test/modules/track_toast/expected/track_toast.out create mode 100644 src/test/modules/track_toast/sql/track_toast.sql create mode 100644 src/test/modules/track_toast/track_toast.conf diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..fa40befc16 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7668,6 +7668,32 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + track_toast (boolean) + + track_toast configuration parameter + + + + +Enables tracking of TOAST activities. +Compressions and externalizations are tracked. +The default is off. +Only superusers can change this setting. + + + + +Be aware that this feature, depending on the amount of TOASTable columns in +your databases, may significantly increase the size of the statistics files +and the workload of the statistics collector. It is recommended to only +temporarily activate this to assess the right compression and storage method +for (a) column(s). + + + + + stats_temp_directory (string) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 62f2a3332b..32d7818096 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -610,6 +610,17 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser yet included in pg_stat_user_functions). + + pg_stat_toastpg_stat_toast + + One row for each column that has ever been TOASTed (compressed and/or externalized). + Showing the number of externalizations, compression attempts / successes, compressed and + uncompressed sizes etc. + + pg_stat_toast for details. + + + pg_stat_slrupg_stat_slru One row per SLRU, showing statistics of operations. See @@ -4969,6 +4980,158 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i + + pg_stat_toast + + + pg_stat_toast + + + + The pg_stat_toast view will contain + one row for each column of variable size that has been TOASTed since + the last statistics reset. The parameter + controls whether TOAST activities are tracked or not. + + + + pg_stat_toast View + + + + + Column Type + + + Description + + + + + + + + schemaname name + + + Name of the schema the relation is in + + + + + + reloid oid + + + OID of the relation + + + + + + attnum int + + + Attribute (column) number in the relation + + + + + + relname name + + + Name of the relation + + + + + + attname name + + + Name of the attribute (column) + + + + + + storagemethod char + + + Storage method of the attribute + + + + + + externalized bigint + + +
Re: Clarify planner_hook calling convention
"Andrey V. Lepikhov" writes: > planner hook is frequently used in monitoring and advising extensions. Yeah. > The call to this hook is implemented in the way, that the > standard_planner routine must be called at least once in the hook's call > chain. > But, as I see in [1], it should allow us "... replace the planner > altogether". > In such situation it haven't sense to call standard_planner at all. That's possible in theory, but who's going to do it in practice? There is a monstrous amount of code you'd have to replace. Moreover, if you felt compelled to do it, it's likely because you are making fundamental changes elsewhere too, which means you are more likely going to end up with a fork not an extension. > But, maybe more simple solution is to describe requirements to such kind > of extensions in the code and documentation (See patch in attachment)? > + * 2. If your extension implements some planning activity, write in the > extension > + * docs a requirement to set the extension at the begining of shared > libraries list. This advice seems pretty unhelpful. If more than one extension is getting into the planner_hook, they can't all be first. (Also, largely the same issue applies to very many of our other hooks.) regards, tom lane
Re: Remove inconsistent quotes from date_part error
Updated patch attached.
On Mon, Jan 3, 2022 at 10:26 AM Nikhil Benesch wrote:
>
> On Mon, Jan 3, 2022 at 10:20 AM Tom Lane wrote:
> > BTW, if you want to get rid of the quotes, I think that something
> > else has to be done to set off the type name from the rest. In
> > this instance someone might think that we're complaining about a
> > "time zone unit", whatever that is. I suggest swapping it around to
> >
> > units \"%s\" not recognized for type %s
> >
> > Also, personally, I'd write unit not units, but that's
> > more debatable.
>
> Your suggestion sounds good to me. I'll update the patch with that.
>
> Not that it changes anything, I think, but the wording ambiguity you
> mention is present today in the timestamptz error message:
>
> benesch=> select extract('nope' from now());
> ERROR: timestamp with time zone units "nope" not recognized
v2-0001-Improve-unsupported-units-error-for-extract-date_.patch
Description: Binary data
Re: [PATCH] pg_stat_toast v0.4
Am 21.12.21 um 13:51 schrieb kuroda.hay...@fujitsu.com: Dear Gunnar, Hayato-san, all, thanks for the feedback! * gathering timing information Here is a minor comment: I'm not sure when we should start to measure time. If you want to record time spent for compressing, toast_compress_datum() should be sandwiched by INSTR_TIME_SET_CURRENT() and caclurate the time duration. Currently time_spent is calcurated in the pgstat_report_toast_activity(), but this have a systematic error. If you want to record time spent for inserting/updating, however, I think we should start measuring at the upper layer, maybe heap_toast_insert_or_update(). Yes, both toast_compress_datum() and toast_save_datum() are sandwiched the way you mentioned, as that's exactly what we want to measure (time spent doing compression and/or externalizing data). Implementation-wise, I (again) took "track_functions" as a template there, assuming that jumping into pgstat_report_toast_activity() and only then checking if "track_toast = on" isn't too costly (we call pgstat_init_function_usage() and pgstat_end_function_usage() a _lot_). I did miss though that INSTR_TIME_SET_CURRENT(time_spent); should be called right after entering pgstat_report_toast_activity(), as that might need additional clock ticks for setting up the hash etc. That's fixed now. What I can't assess is the cost of the unconditional call to INSTR_TIME_SET_CURRENT(start_time) in both toast_tuple_try_compression() and toast_tuple_externalize(). Would it be wise (cheaper) to add a check like if (pgstat_track_toast) before querying the system clock? Any hints on how to write meaningful tests would be much appreciated! I.e., will a test I searched hints from PG sources, and I thought that modules/ subdirectory can be used. Following is the example: https://github.com/postgres/postgres/tree/master/src/test/modules/commit_ts I attached a patch to create a new test. Could you rewrite the sql and the output file? Thanks for the kickstart ;-) Some tests (as meaningful as they may get, I'm afraid) are now in src/test/modules/track_toast/. "make check-world" executes them successfully, although only after I introduced a "SELECT pg_sleep(1);" to them. pg_stat_toast_v0.4.patch attached. Best regards, -- Gunnar "Nick" Bluth Eimermacherweg 106 D-48159 Münster Mobil +49 172 8853339 Email: gunnar.bl...@pro-open.de __ "Ceterum censeo SystemD esse delendam" - Catodiff --git a/.gitignore b/.gitignore index 794e35b73c..ca2ee84742 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,8 @@ win32ver.rc *.exe lib*dll.def lib*.pc +tags +/.vscode # Local excludes in root directory /GNUmakefile @@ -42,3 +44,5 @@ lib*.pc /Debug/ /Release/ /tmp_install/ +.gitignore +pg_stat_toast.* diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..fa40befc16 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -7668,6 +7668,32 @@ COPY postgres_log FROM '/full/path/to/logfile.csv' WITH csv; + + track_toast (boolean) + + track_toast configuration parameter + + + + +Enables tracking of TOAST activities. +Compressions and externalizations are tracked. +The default is off. +Only superusers can change this setting. + + + + +Be aware that this feature, depending on the amount of TOASTable columns in +your databases, may significantly increase the size of the statistics files +and the workload of the statistics collector. It is recommended to only +temporarily activate this to assess the right compression and storage method +for (a) column(s). + + + + + stats_temp_directory (string) diff --git a/doc/src/sgml/monitoring.sgml b/doc/src/sgml/monitoring.sgml index 62f2a3332b..32d7818096 100644 --- a/doc/src/sgml/monitoring.sgml +++ b/doc/src/sgml/monitoring.sgml @@ -610,6 +610,17 @@ postgres 27093 0.0 0.0 30096 2752 ?Ss 11:34 0:00 postgres: ser yet included in pg_stat_user_functions). + + pg_stat_toastpg_stat_toast + + One row for each column that has ever been TOASTed (compressed and/or externalized). + Showing the number of externalizations, compression attempts / successes, compressed and + uncompressed sizes etc. + + pg_stat_toast for details. + + + pg_stat_slrupg_stat_slru One row per SLRU, showing statistics of operations. See @@ -4969,6 +4980,158 @@ SELECT pid, wait_event_type, wait_event FROM pg_stat_activity WHERE wait_event i + + pg_stat_toast + + + pg_stat_toast + + + + The pg_stat_toast view will contain + one row for each column of variable size that has been TOASTed since + the last statistics
Re: Refactoring of compression options in pg_basebackup
Hi,
‐‐‐ Original Message ‐‐‐
On Saturday, December 18th, 2021 at 12:29 PM, Michael Paquier
wrote:
>Hi all,
>(Added Georgios in CC.)
thank you for the shout out.
>When working on the support of LZ4 for pg_receivewal, walmethods.c has
>gained an extra parameter for the compression method. It gets used on
>the DirectoryMethod instead of the compression level to decide which
>type of compression is used. One thing that I left out during this
>previous work is that the TarMethod also gained knowledge of this
>compression method, but we still use the compression level to check if
>tars should be compressed or not.
>
>This is wrong on multiple aspects. First, this is not consistent with
>the directory method, making walmethods.c harder to figure out.
>Second, this is not extensible if we want to introduce more
>compression methods in pg_basebackup, like LZ4. This reflects on the
>options used by pg_receivewal and pg_basebackup, that are not
>inconsistent as well.
Agreed with all the above.
>The attached patch refactors the code of pg_basebackup and the
>TarMethod of walmethods.c to use the compression method where it
>should, splitting entirely the logic related the compression level.
Thanks.
>This is one step toward the introduction of LZ4 in pg_basebackup, but
>this refactoring is worth doing on its own, hence a separate thread to
>deal with this problem first. The options of pg_basebackup are
>reworked to be consistent with pg_receivewal, as follows:
>- --compress ranges now from 1 to 9, instead of 0 to 9.
>- --compression-method={none,gzip} is added, the default is none, same
>as HEAD.
>- --gzip/-z has the same meaning as before, being just a synonym of
>--compression-method=gzip with the default compression level of ZLIB
>assigned if there is no --compress.
Indeed this is consistent with pg_receivewal. It gets my +1.
>One more thing that I have noticed while hacking this stuff is that we
>have no regression tests for gzip with pg_basebackup, so I have added
>some that are skipped when not compiling the code with ZLIB.
As far as the code is concerned, I have a minor nitpick.
+ if (compression_method == COMPRESSION_NONE)
+ streamer = bbstreamer_plain_writer_new(archive_filename,
+ archive_file);
#ifdef HAVE_LIBZ
- if (compresslevel != 0)
+ else if (compression_method == COMPRESSION_GZIP)
{
strlcat(archive_filename, ".gz", sizeof(archive_filename));
streamer = bbstreamer_gzip_writer_new(archive_filename,
archive_file,
compresslevel);
}
- else
#endif
- streamer = bbstreamer_plain_writer_new(archive_filename,
- archive_file);
-
+ else
+ {
+ Assert(false); /* not reachable */
+ }
The above block moves the initialization of 'streamer' within two conditional
blocks. Despite this being correct, it is possible that some compilers will
complain for lack of initialization of 'streamer' when it is eventually used a
bit further ahead in:
if (must_parse_archive)
streamer = bbstreamer_tar_archiver_new(streamer);
I propose to initialize streamer to NULL when declared at the top of
CreateBackupStreamer().
As far as the tests are concerned, I think that 2 too many tests are skipped
when HAVE_LIBZ is not defined to be 1. The patch reads:
+Check ZLIB compression if available.
+SKIP:
+{
+ skip "postgres was not built with ZLIB support", 5
+ if (!check_pg_config("#define HAVE_LIBZ 1"));
+
+ $node->command_ok(
+ [
+ 'pg_basebackup','-D',
+ "$tempdir/backup_gzip", '--compression-method',
+ 'gzip', '--compress', '1', '--no-sync', '--format', 't'
+ ],
+ 'pg_basebackup with --compress and --compression-method=gzip');
+
+ # Verify that the stored files are generated with their expected
+ # names.
+ my @zlib_files = glob "$tempdir/backup_gzip/*.tar.gz";
+ is(scalar(@zlib_files), 2,
+ "two files created with gzip (base.tar.gz and pg_wal.tar.gz)");
+
+ # Check the integrity of the files generated.
+ my $gzip = $ENV{GZIP_PROGRAM};
+ skip "program gzip is not found in your system", 1
+ if ( !defined $gzip
+ || $gzip eq ''
+ || system_log($gzip, '--version') != 0);
+
+ my $gzip_is_valid = system_log($gzip, '--test', @zlib_files);
+ is($gzip_is_valid, 0, "gzip verified the integrity of compressed data");
+ rmtree("$tempdir/backup_gzip");
+}
You can see that after the check_pg_config() test, only 3 tests follow,
namely:
* $node->command_ok()
* is(scalar(), ...)
* is($gzip_is_valid, ...)
>Opinions?
Other than the minor issues above, I think this is a solid improvement. +1
>--
>Michael
Cheers,
//Georgios
Re: Remove inconsistent quotes from date_part error
On Mon, Jan 3, 2022 at 10:20 AM Tom Lane wrote:
> BTW, if you want to get rid of the quotes, I think that something
> else has to be done to set off the type name from the rest. In
> this instance someone might think that we're complaining about a
> "time zone unit", whatever that is. I suggest swapping it around to
>
> units \"%s\" not recognized for type %s
>
> Also, personally, I'd write unit not units, but that's
> more debatable.
Your suggestion sounds good to me. I'll update the patch with that.
Not that it changes anything, I think, but the wording ambiguity you
mention is present today in the timestamptz error message:
benesch=> select extract('nope' from now());
ERROR: timestamp with time zone units "nope" not recognized
Re: Remove inconsistent quotes from date_part error
Nikhil Benesch writes:
> - errmsg("\"time with time zone\" units \"%s\" not
> recognized",
> + errmsg("time with time zone units \"%s\" not
> recognized",
> [ etc ]
BTW, if you want to get rid of the quotes, I think that something
else has to be done to set off the type name from the rest. In
this instance someone might think that we're complaining about a
"time zone unit", whatever that is. I suggest swapping it around to
units \"%s\" not recognized for type %s
Also, personally, I'd write unit not units, but that's
more debatable.
regards, tom lane
Re: Remove inconsistent quotes from date_part error
On Mon, Jan 3, 2022 at 10:11 AM Tom Lane wrote: > Yeah, but we've been slowly converting to that method to reduce the number > of distinct translatable strings for error messages. If doing so here > would cut the labor for translators, I'm for it. Great! I'll update the patch. Thanks for confirming.
Re: Remove inconsistent quotes from date_part error
Nikhil Benesch writes: > I could find only a tiny smattering of examples where format_type_be() > is invoked with a constant OID. In almost all error messages where the > type is statically known, it seems the type name is hardcoded into the > error message rather than generated via format_type_be(). For example, > all of the "TYPE out of range" errors. Yeah, but we've been slowly converting to that method to reduce the number of distinct translatable strings for error messages. If doing so here would cut the labor for translators, I'm for it. regards, tom lane
Re: Column Filtering in Logical Replication
On 2022-Jan-03, Justin Pryzby wrote:
> Yes, I know both paths are hit now that it uses GetBool.
>
> What I'm wondering is why tests didn't fail when one path wasn't hit - when it
> said am_partition=DatumGetChar(); if (!am_partition){}
Ah!
> I suppose it's because the am_partition=true case correctly handles
> nonpartitions.
>
> Maybe the !am_partition case should be removed, and add a comment that
> pg_partition_tree(pg_partition_root(%u))) also handles non-partitions.
> Or maybe that's inefficient...
Hmm, that doesn't sound true. Running the query manually, you get an
empty list if you use pg_partition_tree(pg_partition_root) with a
non-partition. Maybe what was happening is that all columns were being
transmitted instead of only the required columns. Maybe you're right
that the test isn't complete enough.
--
Álvaro Herrera PostgreSQL Developer — https://www.EnterpriseDB.com/
Re: Index-only scans vs. partially-retrievable indexes
Andrey Borodin writes: > I've tried to toy with the patch and remembered one related caveat. > If we have index for both returnable and nonreturnable attributes, IOS will > not be choosen: > postgres=# create index on t using gist(a gist_trgm_ops) include (a); > postgres=# explain select * from t where a like 'z'; > QUERY PLAN > - > Index Scan using t_a_a1_idx on t (cost=0.12..8.14 rows=1 width=32) >Index Cond: (a ~~ 'z'::text) > (2 rows) This case is improved by 0002, no? regards, tom lane
Re: Remove inconsistent quotes from date_part error
On Mon, Jan 3, 2022 at 3:17 AM Michael Paquier wrote: > However, there is a specific routine called format_type_be() aimed at > formatting type names for error strings. If you switch to that, my > guess is that this makes the error messages of time/timetz and > timestamp/timestamptz/interval more consistent, while reducing the > effort of translation because we'd finish with the same base error > string, as of "%s units \"%s\" not recognized". I could find only a tiny smattering of examples where format_type_be() is invoked with a constant OID. In almost all error messages where the type is statically known, it seems the type name is hardcoded into the error message rather than generated via format_type_be(). For example, all of the "TYPE out of range" errors. I'm happy to rework the patch to use format_type_be(), but wanted to double check first that it is the preferred approach in this situation.
Re: Column Filtering in Logical Replication
On Mon, Jan 03, 2022 at 11:31:39AM -0300, Alvaro Herrera wrote:
> > Did the DatumGetBool issue expose a deficiency in testing ?
> > I think the !am_partition path was never being hit.
>
> Hmm, the TAP test creates a subscription that contains both types of
> tables. I tried adding an assert for each case, and they were both hit
> on running the test.
Yes, I know both paths are hit now that it uses GetBool.
What I'm wondering is why tests didn't fail when one path wasn't hit - when it
said am_partition=DatumGetChar(); if (!am_partition){}
I suppose it's because the am_partition=true case correctly handles
nonpartitions.
Maybe the !am_partition case should be removed, and add a comment that
pg_partition_tree(pg_partition_root(%u))) also handles non-partitions.
Or maybe that's inefficient...
--
Justin
Re: Foreign key joins revisited
On Wed, Dec 29, 2021, at 10:46, Peter Eisentraut wrote: >In the 1990s, there were some SQL drafts that included syntax like Do you remember if it was in the beginning/middle/end of the 1990s? I will start the work of going through all drafts tomorrow. /Joel
Re: Column Filtering in Logical Replication
On 2021-Dec-31, Justin Pryzby wrote:
> > @@ -5963,8 +5967,20 @@ describePublications(const char *pattern)
> > {
> > +"
> > CASE WHEN pr.prattrs IS NOT NULL THEN\n"
> > +"
> > pg_catalog.array_to_string"
> > +
> > "(ARRAY(SELECT attname\n"
> > +"
> >FROM pg_catalog.generate_series(0,
> > pg_catalog.array_upper(pr.prattrs::int[], 1)) s,\n"
> > +"
> > pg_catalog.pg_attribute\n"
> > +"
> > WHERE attrelid = c.oid AND attnum = prattrs[s]), ', ')\n"
> > +"
> > ELSE NULL END AS columns");
> I suppose this should use pr.prattrs::pg_catalog.int2[] ?
True. Changed that.
Another change in this v15 is that I renamed the test file from ".patch"
to ".pl". I suppose I mistyped the extension when renumbering from 021
to 028.
> Did the DatumGetBool issue expose a deficiency in testing ?
> I think the !am_partition path was never being hit.
Hmm, the TAP test creates a subscription that contains both types of
tables. I tried adding an assert for each case, and they were both hit
on running the test.
--
Álvaro Herrera 39°49'30"S 73°17'W — https://www.EnterpriseDB.com/
"La persona que no quería pecar / estaba obligada a sentarse
en duras y empinadas sillas/ desprovistas, por cierto
de blandos atenuantes" (Patricio Vogel)
>From ad2766290e7011481813ce24c1947bff70415211 Mon Sep 17 00:00:00 2001
From: Alvaro Herrera
Date: Mon, 6 Sep 2021 10:34:29 -0300
Subject: [PATCH v15] Support column lists for logical replication of tables
Add the capability of specifying a column list for individual tables as
part of a publication. Columns not in the list are not published. This
enables replicating to a table with only a subset of the columns.
If no column list is specified, all the columns are replicated, as
previously
Author: Rahila Syed
Discussion: https://postgr.es/m/CAH2L28vddB_NFdRVpuyRBJEBWjz4BSyTB=_ektnrh8nj1jf...@mail.gmail.com
---
doc/src/sgml/protocol.sgml | 4 +-
doc/src/sgml/ref/alter_publication.sgml | 20 +-
doc/src/sgml/ref/create_publication.sgml| 11 +-
src/backend/catalog/pg_publication.c| 306 +++-
src/backend/commands/publicationcmds.c | 67 -
src/backend/commands/tablecmds.c| 79 -
src/backend/nodes/copyfuncs.c | 1 +
src/backend/nodes/equalfuncs.c | 1 +
src/backend/parser/gram.y | 60 +++-
src/backend/replication/logical/proto.c | 66 +++--
src/backend/replication/logical/tablesync.c | 120 +++-
src/backend/replication/pgoutput/pgoutput.c | 78 -
src/bin/pg_dump/pg_dump.c | 41 ++-
src/bin/pg_dump/pg_dump.h | 1 +
src/bin/psql/describe.c | 26 +-
src/bin/psql/tab-complete.c | 2 +
src/include/catalog/pg_publication.h| 5 +
src/include/catalog/pg_publication_rel.h| 3 +
src/include/nodes/parsenodes.h | 4 +-
src/include/replication/logicalproto.h | 6 +-
src/test/regress/expected/publication.out | 33 ++-
src/test/regress/sql/publication.sql| 20 +-
src/test/subscription/t/028_column_list.pl | 164 +++
23 files changed, 1034 insertions(+), 84 deletions(-)
create mode 100644 src/test/subscription/t/028_column_list.pl
diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml
index 34a7034282..5bc2e7a591 100644
--- a/doc/src/sgml/protocol.sgml
+++ b/doc/src/sgml/protocol.sgml
@@ -6877,7 +6877,9 @@ Relation
-Next, the following message part appears for each column (except generated columns):
+Next, the following message part appears for each column (except
+generated columns and other columns that don't appear in the column
+filter list, for tables that have one):
diff --git a/doc/src/sgml/ref/alter_publication.sgml b/doc/src/sgml/ref/alter_publication.sgml
index bb4ef5e5e2..16a12b44b9 100644
--- a/doc/src/sgml/ref/alter_publication.sgml
+++ b/doc/src/sgml/ref/alter_publication.sgml
@@ -25,12 +25,13 @@ ALTER PUBLICATION name ADD name SET publication_object [, ...]
ALTER PUBLICATION name DROP publication_object [, ...]
ALTER PUBLICATION name SET ( publication_parameter [= value] [, ... ] )
+ALTER PUBLICATION name ALTER TABLE publication_object SET COLUMNS { ( name [, ...] ) | ALL }
ALTER PUBLICATION name OWNER TO { new_owner | CURRENT_ROLE | CURRENT_USER | SESSION_USER }
ALTER PUBLICATION name
Re: refactoring basebackup.c
On 11/22/21 11:05 PM, Jeevan Ladhe wrote: Please find the lz4 compression patch here that basically has: One small issue, in the "pg_basebackup --help", we are not displaying lz4 value under --server-compression option [edb@tusharcentos7-v14 bin]$ ./pg_basebackup --help | grep server-compression --server-compression=none|gzip|gzip[1-9] -- regards,tushar EnterpriseDB https://www.enterprisedb.com/ The Enterprise PostgreSQL Company
Re: Synchronizing slots from primary to standby
Here is an updated patch to fix some build failures. No feature changes. On 14.12.21 23:12, Peter Eisentraut wrote: On 31.10.21 11:08, Peter Eisentraut wrote: I want to reactivate $subject. I took Petr Jelinek's patch from [0], rebased it, added a bit of testing. It basically works, but as mentioned in [0], there are various issues to work out. The idea is that the standby runs a background worker to periodically fetch replication slot information from the primary. On failover, a logical subscriber would then ideally find up-to-date replication slots on the new publisher and can just continue normally. So, again, this isn't anywhere near ready, but there is already a lot here to gather feedback about how it works, how it should work, how to configure it, and how it fits into an overall replication and HA architecture. Here is an updated patch. The main changes are that I added two configuration parameters. The first, synchronize_slot_names, is set on the physical standby to specify which slots to sync from the primary. By default, it is empty. (This also fixes the recovery test failures that I had to disable in the previous patch version.) The second, standby_slot_names, is set on the primary. It holds back logical replication until the listed physical standbys have caught up. That way, when failover is necessary, the promoted standby is not behind the logical replication consumers. In principle, this works now, I think. I haven't made much progress in creating more test cases for this; that's something that needs more attention. It's worth pondering what the configuration language for standby_slot_names should be. Right now, it's just a list of slots that all need to be caught up. More complicated setups are conceivable. Maybe you have standbys S1 and S2 that are potential failover targets for logical replication consumers L1 and L2, and also standbys S3 and S4 that are potential failover targets for logical replication consumers L3 and L4. Viewed like that, this setting could be a replication slot setting. The setting might also have some relationship with synchronous_standby_names. Like, if you have synchronous_standby_names set, then that's a pretty good indication that you also want some or all of those standbys in standby_slot_names. (But note that one is slots and one is application names.) So there are a variety of possibilities. From ec00dc6ab8bafefc00e9b1c78ac9348b643b8a87 Mon Sep 17 00:00:00 2001 From: Peter Eisentraut Date: Mon, 3 Jan 2022 14:43:36 +0100 Subject: [PATCH v3] Synchronize logical replication slots from primary to standby Discussion: https://www.postgresql.org/message-id/flat/514f6f2f-6833-4539-39f1-96cd1e011f23%40enterprisedb.com --- doc/src/sgml/config.sgml | 34 ++ src/backend/commands/subscriptioncmds.c | 4 +- src/backend/postmaster/bgworker.c | 3 + .../libpqwalreceiver/libpqwalreceiver.c | 94 src/backend/replication/logical/Makefile | 1 + src/backend/replication/logical/launcher.c| 202 ++--- .../replication/logical/reorderbuffer.c | 85 src/backend/replication/logical/slotsync.c| 412 ++ src/backend/replication/logical/tablesync.c | 13 +- src/backend/replication/repl_gram.y | 32 +- src/backend/replication/repl_scanner.l| 1 + src/backend/replication/slotfuncs.c | 2 +- src/backend/replication/walsender.c | 196 - src/backend/utils/activity/wait_event.c | 3 + src/backend/utils/misc/guc.c | 23 + src/backend/utils/misc/postgresql.conf.sample | 2 + src/include/commands/subscriptioncmds.h | 3 + src/include/nodes/nodes.h | 1 + src/include/nodes/replnodes.h | 9 + src/include/replication/logicalworker.h | 9 + src/include/replication/slot.h| 4 +- src/include/replication/walreceiver.h | 16 + src/include/replication/worker_internal.h | 8 +- src/include/utils/wait_event.h| 1 + src/test/recovery/t/030_slot_sync.pl | 58 +++ 25 files changed, 1146 insertions(+), 70 deletions(-) create mode 100644 src/backend/replication/logical/slotsync.c create mode 100644 src/test/recovery/t/030_slot_sync.pl diff --git a/doc/src/sgml/config.sgml b/doc/src/sgml/config.sgml index afbb6c35e3..2b2a21a251 100644 --- a/doc/src/sgml/config.sgml +++ b/doc/src/sgml/config.sgml @@ -4406,6 +4406,23 @@ Primary Server + + standby_slot_names (string) + + standby_slot_names configuration parameter + + + + +List of physical replication slots that logical replication waits for. +If a logical replication connection is meant to switch to a physical +standby after the standby is promoted, the physical replication slot +for the standby should be
Re: Add Boolean node
On 03.01.22 12:04, Peter Eisentraut wrote:
On 27.12.21 10:02, Peter Eisentraut wrote:
This patch adds a new node type Boolean, to go alongside the "value"
nodes Integer, Float, String, etc. This seems appropriate given that
Boolean values are a fundamental part of the system and are used a lot.
Before, SQL-level Boolean constants were represented by a string with
a cast, and internal Boolean values in DDL commands were usually
represented by Integer nodes. This takes the place of both of these
uses, making the intent clearer and having some amount of type safety.
Here is an update of this patch set based on the feedback. First, I
added a patch that makes some changes in AlterRole() that my original
patch might have broken or at least made more confusing. Unlike in
CreateRole(), we use three-valued logic here, so that a variable like
issuper would have 0 = no, 1 = yes, -1 = not specified, keep previous
value. I'm simplifying this, by instead using the dissuper etc.
variables to track whether a setting was specified. This makes
everything a bit simpler and makes the subsequent patch easier.
Second, I added the suggest by Tom Lane to rename to fields in the
used-to-be-Value nodes to be different in each node type (ival, fval,
etc.). I agree that this makes things a bit cleaner and reduces the
changes of mixups.
And third, the original patch that introduces the Boolean node with some
small changes based on the feedback.
Another very small update, attempting to appease the cfbot.From 8d5f7f330c0824c862a3c0d8ce5cd3d578e22d82 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 3 Jan 2022 09:37:12 +0100
Subject: [PATCH v3 1/3] Refactor AlterRole()
Get rid of the three-valued logic for the Boolean variables to track
whether the value was been specified and what the new value should be.
Instead, we can use the "dfoo" variables to determine whether the
value was specified and should be applied. This was already done in
some cases, so this makes this more uniform and removes one layer of
indirection.
---
src/backend/commands/user.c | 97 +
1 file changed, 35 insertions(+), 62 deletions(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index aa69821be4..0aae87ff4a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -501,20 +501,12 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
new_tuple;
Form_pg_authid authform;
ListCell *option;
- char *rolename = NULL;
+ char *rolename;
char *password = NULL;/* user password */
- int issuper = -1; /* Make the user a superuser? */
- int inherit = -1; /* Auto inherit privileges? */
- int createrole = -1;/* Can this user create
roles? */
- int createdb = -1; /* Can the user create
databases? */
- int canlogin = -1; /* Can this user login? */
- int isreplication = -1; /* Is this a replication
role? */
int connlimit = -1; /* maximum connections allowed
*/
- List *rolemembers = NIL; /* roles to be added/removed */
char *validUntil = NULL; /* time the login is valid until */
Datum validUntil_datum; /* same, as timestamptz Datum */
boolvalidUntil_null;
- int bypassrls = -1;
DefElem*dpassword = NULL;
DefElem*dissuper = NULL;
DefElem*dinherit = NULL;
@@ -610,18 +602,6 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (dpassword && dpassword->arg)
password = strVal(dpassword->arg);
- if (dissuper)
- issuper = intVal(dissuper->arg);
- if (dinherit)
- inherit = intVal(dinherit->arg);
- if (dcreaterole)
- createrole = intVal(dcreaterole->arg);
- if (dcreatedb)
- createdb = intVal(dcreatedb->arg);
- if (dcanlogin)
- canlogin = intVal(dcanlogin->arg);
- if (disreplication)
- isreplication = intVal(disreplication->arg);
if (dconnlimit)
{
connlimit = intVal(dconnlimit->arg);
@@ -630,12 +610,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid connection limit: %d",
connlimit)));
}
- if (drolemembers)
- rolemembers = (List *) drolemembers->arg;
if (dvalidUntil)
validUntil = strVal(dvalidUntil->arg);
- if (dbypassRLS)
- bypassrls = intVal(dbypassRLS->arg);
/*
* Scan the pg_authid relation to be certain the user exists.
@@ -654,21
Re: daitch_mokotoff module
Tom Lane writes:
> Thomas Munro writes:
>> Erm, it looks like something weird is happening somewhere in cfbot's
>> pipeline, because Dag's patch says:
>
>> +SELECT daitch_mokotoff('Straßburg');
>> + daitch_mokotoff
>> +-
>> + 294795
>> +(1 row)
>
> ... so, that test case is guaranteed to fail in non-UTF8 encodings,
> I suppose? I wonder what the LANG environment is in that cfbot
> instance.
>
> (We do have methods for dealing with non-ASCII test cases, but
> I can't see that this patch is using any of them.)
>
> regards, tom lane
>
I naively assumed that tests would be run in an UTF8 environment.
Running "ack -l '[\x80-\xff]'" in the contrib/ directory reveals that
two other modules are using UTF8 characters in tests - citext and
unaccent.
The citext tests seem to be commented out - "Multibyte sanity
tests. Uncomment to run."
Looking into the unaccent module, I don't quite understand how it will
work with various encodings, since it doesn't seem to decode its input -
will it fail if run under anything but ASCII or UTF8?
In any case, I see that unaccent.sql starts as follows:
CREATE EXTENSION unaccent;
-- must have a UTF8 database
SELECT getdatabaseencoding();
SET client_encoding TO 'UTF8';
Would doing the same thing in fuzzystrmatch.sql fix the problem with
failing tests? Should I prepare a new patch?
Best regards
Dag Lem
Remove extra spaces
Hi, While browsing the code, noticed the extra spaces after the function name. Removed the same in the attached patch. -- -- Thanks & Regards, Suraj kharage, edbpostgres.com remove_space_from_numeric_function.patch Description: Binary data
Re: libpq compression (part 2)
> Maybe you should reset the streams between each compression message (even if > it's using the same compression algorithm). This might allow better > compression. AFAIK on the contrary - longer data sequence usually compresses better. The codec can use knowledge about prior data to better compress current bytes. Best regards, Andrey Borodin.
Re: Index-only scans vs. partially-retrievable indexes
> regression=# explain select * from t where lower(a) like 'z'; > QUERY PLAN > -- > Index Only Scan using t_lower_a_idx on t (cost=0.14..28.27 rows=7 width=32) > Index Cond: ((lower(a)) ~~ 'z'::text) > (2 rows) > I've tried to toy with the patch and remembered one related caveat. If we have index for both returnable and nonreturnable attributes, IOS will not be choosen: postgres=# create index on t using gist(a gist_trgm_ops) include (a); postgres=# explain select * from t where a like 'z'; QUERY PLAN - Index Scan using t_a_a1_idx on t (cost=0.12..8.14 rows=1 width=32) Index Cond: (a ~~ 'z'::text) (2 rows) But with index create index on t using gist(lower(a) gist_trgm_ops) include (a); I observe IOS for select * from t where lower(a) like 'z'; So lossiness of opclass kind of "defeats" returnable attribute. But lossiness of expression does not. I don't feel condifent in surrounding code to say is it a bug or just a lack of feature. But maybe we would like to have equal behavior in both cases... Thanks! Best regards, Andrey Borodin.
Re: Add Boolean node
On 27.12.21 10:02, Peter Eisentraut wrote:
This patch adds a new node type Boolean, to go alongside the "value"
nodes Integer, Float, String, etc. This seems appropriate given that
Boolean values are a fundamental part of the system and are used a lot.
Before, SQL-level Boolean constants were represented by a string with
a cast, and internal Boolean values in DDL commands were usually
represented by Integer nodes. This takes the place of both of these
uses, making the intent clearer and having some amount of type safety.
Here is an update of this patch set based on the feedback. First, I
added a patch that makes some changes in AlterRole() that my original
patch might have broken or at least made more confusing. Unlike in
CreateRole(), we use three-valued logic here, so that a variable like
issuper would have 0 = no, 1 = yes, -1 = not specified, keep previous
value. I'm simplifying this, by instead using the dissuper etc.
variables to track whether a setting was specified. This makes
everything a bit simpler and makes the subsequent patch easier.
Second, I added the suggest by Tom Lane to rename to fields in the
used-to-be-Value nodes to be different in each node type (ival, fval,
etc.). I agree that this makes things a bit cleaner and reduces the
changes of mixups.
And third, the original patch that introduces the Boolean node with some
small changes based on the feedback.From 8d5f7f330c0824c862a3c0d8ce5cd3d578e22d82 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 3 Jan 2022 09:37:12 +0100
Subject: [PATCH v2 1/3] Refactor AlterRole()
Get rid of the three-valued logic for the Boolean variables to track
whether the value was been specified and what the new value should be.
Instead, we can use the "dfoo" variables to determine whether the
value was specified and should be applied. This was already done in
some cases, so this makes this more uniform and removes one layer of
indirection.
---
src/backend/commands/user.c | 97 +
1 file changed, 35 insertions(+), 62 deletions(-)
diff --git a/src/backend/commands/user.c b/src/backend/commands/user.c
index aa69821be4..0aae87ff4a 100644
--- a/src/backend/commands/user.c
+++ b/src/backend/commands/user.c
@@ -501,20 +501,12 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
new_tuple;
Form_pg_authid authform;
ListCell *option;
- char *rolename = NULL;
+ char *rolename;
char *password = NULL;/* user password */
- int issuper = -1; /* Make the user a superuser? */
- int inherit = -1; /* Auto inherit privileges? */
- int createrole = -1;/* Can this user create
roles? */
- int createdb = -1; /* Can the user create
databases? */
- int canlogin = -1; /* Can this user login? */
- int isreplication = -1; /* Is this a replication
role? */
int connlimit = -1; /* maximum connections allowed
*/
- List *rolemembers = NIL; /* roles to be added/removed */
char *validUntil = NULL; /* time the login is valid until */
Datum validUntil_datum; /* same, as timestamptz Datum */
boolvalidUntil_null;
- int bypassrls = -1;
DefElem*dpassword = NULL;
DefElem*dissuper = NULL;
DefElem*dinherit = NULL;
@@ -610,18 +602,6 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
if (dpassword && dpassword->arg)
password = strVal(dpassword->arg);
- if (dissuper)
- issuper = intVal(dissuper->arg);
- if (dinherit)
- inherit = intVal(dinherit->arg);
- if (dcreaterole)
- createrole = intVal(dcreaterole->arg);
- if (dcreatedb)
- createdb = intVal(dcreatedb->arg);
- if (dcanlogin)
- canlogin = intVal(dcanlogin->arg);
- if (disreplication)
- isreplication = intVal(disreplication->arg);
if (dconnlimit)
{
connlimit = intVal(dconnlimit->arg);
@@ -630,12 +610,8 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
(errcode(ERRCODE_INVALID_PARAMETER_VALUE),
errmsg("invalid connection limit: %d",
connlimit)));
}
- if (drolemembers)
- rolemembers = (List *) drolemembers->arg;
if (dvalidUntil)
validUntil = strVal(dvalidUntil->arg);
- if (dbypassRLS)
- bypassrls = intVal(dbypassRLS->arg);
/*
* Scan the pg_authid relation to be certain the user exists.
@@ -654,21 +630,21 @@ AlterRole(ParseState *pstate, AlterRoleStmt *stmt)
* property. Otherwise, if you don't
Re: An obsolete comment of pg_stat_statements
On Fri, Dec 24, 2021 at 09:02:10PM +0900, Michael Paquier wrote: > Do we really need to have this comment in the function header? The > same is explained a couple of lines down so this feels like a > duplicate, and it is hard to miss it with the code shaped as-is (aka > the relationship between compute_query_id and queryId and the > consequences on what's stored in this case). The simpler the better here. So, I have just removed this comment after thinking more about this. -- Michael signature.asc Description: PGP signature
Re: Remove inconsistent quotes from date_part error
On Sun, Jan 02, 2022 at 11:47:32PM -0500, Nikhil Benesch wrote: > The attached patch corrects a very minor typographical inconsistency > when date_part is invoked with invalid units on time/timetz data vs > timestamp/timestamptz/interval data. Hmm, you are right that this is inconsistent, but I don't think that what you are doing is completely correct either. First, from what I can see from the core code, we don't apply quotes to types in error messages. So your patch is going in the right direction. However, there is a specific routine called format_type_be() aimed at formatting type names for error strings. If you switch to that, my guess is that this makes the error messages of time/timetz and timestamp/timestamptz/interval more consistent, while reducing the effort of translation because we'd finish with the same base error string, as of "%s units \"%s\" not recognized". If we rework this part, we could even rework this error message more. One suggestion I have would be "units of type %s not recognized", for example. > (If stuff like this is too minor to bother with, let me know and I'll > hold off in the future... but since this was pointed out to me I can't > unsee it.) This usually comes down to a case-by-case analysis. Now, in this case, your suggestion looks right to me. -- Michael signature.asc Description: PGP signature
Re: Use MaxLockMode in lock methods initialization
On Mon, Jan 03, 2022 at 02:47:22PM +0800, Julien Rouhaud wrote: > While reading some code around I noticed that b04aeb0a053e7 added a > MaxLockMode > but didn't update the lock methods initialization. It shouldn't make much > difference in the long run but some consistency seems better to me. Makes sense to me. MaxLockMode is here for the same purpose as this initialization area. -- Michael signature.asc Description: PGP signature
