Re: Implementing Incremental View Maintenance
On Fri, 29 Nov 2019 09:50:49 +0900 (JST) Tatsuo Ishii wrote: > > Hi, > > > > Attached is the latest patch (v8) to add support for Incremental View > > Maintenance (IVM). This patch adds OUTER join support in addition > > to the patch (v7) submitted last week in the following post. > > There's a compiler warning: > > matview.c: In function ‘getRteListCell’: > matview.c:2685:9: warning: ‘rte_lc’ may be used uninitialized in this > function [-Wmaybe-uninitialized] > return rte_lc; > ^~ Thanks! I'll fix this. > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Fri, 29 Nov 2019 07:19:44 +0900 (JST) Tatsuo Ishii wrote: > >> As promised, I have created the doc (CREATE MATERIALIZED VIEW manual) > >> patch. > > > > - because the triggers will be invoked. > > + because the triggers will be invoked. We call this form of > > materialized > > + view as "Incremantal materialized View Maintenance" (IVM). > > > > This part seems incorrect to me. Incremental (materialized) View > > Maintenance (IVM) is a way to maintain materialized views and is not a > > word to refer views to be maintained. > > > > However, it would be useful if there is a term referring views which > > can be maintained using IVM. Off the top of my head, we can call this > > Incrementally Maintainable Views (= IMVs), but this might cofusable with > > IVM, so I'll think about that a little more > > But if we introduce IMV, IVM would be used in much less places in the > doc and source code, so less confusion would happen, I guess. Make senses. However, we came to think that "Incrementally Maintainable Materialized Views" (IMMs) would be good. So, how about using this for now? When other better opinions are raised, let's discuss again Regards, Yugo Nagata > > Best regards, > -- > Tatsuo Ishii > SRA OSS, Inc. Japan > English: http://www.sraoss.co.jp/index_en.php > Japanese:http://www.sraoss.co.jp -- Yugo Nagata
Re: Implementing Incremental View Maintenance
The following review on our patch was posted on another thread,
so I quote here. The tab completion is Hoshiai-san's work, so
he will handle this issue.
Regards,
Yugo Nagata.
On Thu, 28 Nov 2019 13:00:05 +0900
nuko yokohama wrote:
> Hi.
>
> I'm using the "Incremental Materialized View Maintenance" patch and have
> reported the following issues.
> (https://commitfest.postgresql.org/25/2138/)
>
> To Suggest a "DROP INCREMENTAL MATERIALIZED VIEW" in psql, but the syntax
> error when you run.
> ("DROP MATERIALIZED VIEW" command can drop Incremental Materialozed view
> normally.)
>
>
> ramendb=# CREATE INCREMENTAL MATERIALIZED VIEW pref_count AS SELECT pref,
> COUNT(pref) FROM shops GROUP BY pref;
> SELECT 48
> ramendb=# \d pref_count
> Materialized view "public.pref_count"
> Column | Type | Collation | Nullable | Default
> ---++---+--+-
> pref | text | | |
> count | bigint | | |
> __ivm_count__ | bigint | | |
>
> ramendb=# DROP IN
> INCREMENTAL MATERIALIZED VIEW INDEX
> ramendb=# DROP INCREMENTAL MATERIALIZED VIEW pref_count;
> 2019-11-27 11:51:03.916 UTC [9759] ERROR: syntax error at or near
> "INCREMENTAL" at character 6
> 2019-11-27 11:51:03.916 UTC [9759] STATEMENT: DROP INCREMENTAL
> MATERIALIZED VIEW pref_count;
> ERROR: syntax error at or near "INCREMENTAL"
> LINE 1: DROP INCREMENTAL MATERIALIZED VIEW pref_count;
> ^
> ramendb=# DROP MATERIALIZED VIEW pref_count ;
> DROP MATERIALIZED VIEW
> ramendb=#
>
>
> Regard.
--
Yugo Nagata
--
Yugo Nagata
Re: To Suggest a "DROP INCREMENTAL MATERIALIZED VIEW" in psql, but the syntax error when you run.
Hello nuko-san,
Thank you for your review!
As Michael commentted, we would like to discuss this on the thread
of the patch, so I quote your review in the following post.
https://www.postgresql.org/message-id/20191129154513.943f4ef05896d7b9d3fed69f%40sraoss.co.jp
Regards,
Yugo Nagata
On Thu, 28 Nov 2019 13:05:33 +0900
Michael Paquier wrote:
> On Thu, Nov 28, 2019 at 01:00:05PM +0900, nuko yokohama wrote:
> > To Suggest a "DROP INCREMENTAL MATERIALIZED VIEW" in psql, but the syntax
> > error when you run.
> > ("DROP MATERIALIZED VIEW" command can drop Incremental Materialozed view
> > normally.)
>
> It seems to me that this is just an issue with the tab completion the
> patch is adding. When reviewing the patch, could you just report such
> issues directly on the thread of the patch? Thanks!
> --
> Michael
--
Yugo Nagata
Re: Implementing Incremental View Maintenance
On Thu, 28 Nov 2019 11:03:33 -0300 Alvaro Herrera wrote: > One thing pending in this development line is how to catalogue aggregate > functions that can be used in incrementally-maintainable views. > I saw a brief mention somewhere that the devels knew it needed to be > done, but I don't see in the thread that they got around to doing it. > Did you guys have any thoughts on how it can be represented in catalogs? > It seems sine-qua-non ... Yes, this is a pending issue. Currently, supported aggregate functions are identified their name, that is, we support aggregate functions named "count", "sum", "avg", "min", or "max". As mentioned before, this is not robust because there might be user-defined aggregates with these names although all built-in aggregates can be used in IVM. In our implementation, the new aggregate values are calculated using "+" and "-" operations for sum and count, "/" for agv, and ">=" / "<=" for min/max. Therefore, if there is a user-defined aggregate on a user-defined type which doesn't support these operators, errors will raise. Obviously, this is a problem. Even if these operators are defined, the semantics of user-defined aggregate functions might not match with the way of maintaining views, and resultant might be incorrect. I think there are at least three options to prevent these problems. In the first option, we support only built-in aggregates which we know able to handle correctly. Supported aggregates can be identified using their OIDs. User-defined aggregates are not supported. I think this is the simplest and easiest way. Second, supported aggregates can be identified using name, like the current implementation, but also it is checked if required operators are defined. In this case, user-defined aggregates are allowed to some extent and we can prevent errors during IVM although aggregates value in view might be incorrect if the semantics doesn't match. Third, we can add a new attribute to pg_aggregate which shows if each aggregate can be used in IVM. We don't need to use names or OIDs list of supported aggregates although we need modification of the system catalogue. Regarding pg_aggregate, now we have aggcombinefn attribute for supporting partial aggregation. Maybe we could use combine functions to calculate new aggregate values in IVM when tuples are inserted into a table. However, in the context of IVM, we also need other function used when tuples are deleted from a table, so we can not use partial aggregation for IVM in the current implementation. This might be another option to implement "inverse combine function"(?) for IVM, but I am not sure it worth. Regards, Yugo Nagata > > -- > Álvaro Herrerahttps://www.2ndQuadrant.com/ > PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Fri, 29 Nov 2019 15:34:52 +0900 Amit Langote wrote: > Thanks a lot for working on this. It's a great (and big!) feature and > I can see that a lot of work has been put into writing this patch. I > started looking at the patch (v8), but as it's quite big: > > 34 files changed, 5444 insertions(+), 69 deletions(-) Thank you for your reviewing the patch! Yes, this is a big patch athough > I'm having a bit of trouble reading through, which I suspect others > may be too. Perhaps, it can be easier for you, as authors, to know > everything that's being changed (added, removed, existing code > rewritten), but certainly not for a reviewer, so I think it would be a > good idea to try to think dividing this into parts. I still don't I agree with you. We also think the need to split the patch and we are considering the way. > have my head wrapped around the topic of materialized view > maintenance, but roughly it looks to me like there are really *two* > features that are being added: > > 1. Add a new method to refresh an MV incrementally; IIUC, there's > already one method that's used by REFRESH MATERIALIZED VIEW > CONCURRENTLY, correct? No, REFRESH MATERIALIZED VIEW CONCURRENTLY is not the way to refresh materialized views. This just acquires weaker locks on views to not prevent SELECT, so this calculate the content of the view completely from scratch. There is no method to incrementally refresh materialized views in the current PostgreSQL. Also, we didn't implement incremental refresh on REFRESH command in this patch. This supports only automatically refresh using triggers. However, we used the code for REFRESH in our IVM implementation, so I think splitting the patch according to this point of view can make sense. > 2. Make the refresh automatic (using triggers on the component tables) > > Maybe, there are even: > > 0. Infrastructure additions Yes, we have a bit modification on the infrastructure, for example, trigger.c. > As you can tell, having the patch broken down like this would allow us > to focus on the finer aspects of each of the problem being solved and > solution being adopted, for example: > > * It would be easier for someone having an expert opinion on how to > implement incremental refresh to have to only look at the patch for > (1). If the new method handles more query types than currently, which > obviously means more code is needed, which in turn entails possibility > of bugs, despite the best efforts. It would be better to get more > eyeballs at this portion of the patch and having it isolated seems > like a good way to attract more eyeballs. > > * Someone well versed in trigger infrastructure can help fine tune the > patch for (2) > > and so on. > > So, please consider giving some thought to this. Agreed. Although I am not sure we will do it as above way, we will consider to split the patch, anyway. Thanks. Regards, Yugo Nagata -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Fri, 29 Nov 2019 18:16:00 +0900 Yugo Nagata wrote: > On Fri, 29 Nov 2019 15:34:52 +0900 > Amit Langote wrote: > > > Thanks a lot for working on this. It's a great (and big!) feature and > > I can see that a lot of work has been put into writing this patch. I > > started looking at the patch (v8), but as it's quite big: > > > > 34 files changed, 5444 insertions(+), 69 deletions(-) > > Thank you for your reviewing the patch! Yes, this is a big patch > athough Sorry, an unfinished line was left... Please ignore this. -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Mon, 02 Dec 2019 10:36:36 +0900 (JST) Tatsuo Ishii wrote: > >> One thing pending in this development line is how to catalogue aggregate > >> functions that can be used in incrementally-maintainable views. > >> I saw a brief mention somewhere that the devels knew it needed to be > >> done, but I don't see in the thread that they got around to doing it. > >> Did you guys have any thoughts on how it can be represented in catalogs? > >> It seems sine-qua-non ... > > > > Yes, this is a pending issue. Currently, supported aggregate functions are > > identified their name, that is, we support aggregate functions named > > "count", > > "sum", "avg", "min", or "max". As mentioned before, this is not robust > > because there might be user-defined aggregates with these names although all > > built-in aggregates can be used in IVM. > > > > In our implementation, the new aggregate values are calculated using "+" and > > "-" operations for sum and count, "/" for agv, and ">=" / "<=" for min/max. > > Therefore, if there is a user-defined aggregate on a user-defined type which > > doesn't support these operators, errors will raise. Obviously, this is a > > problem. Even if these operators are defined, the semantics of user-defined > > aggregate functions might not match with the way of maintaining views, and > > resultant might be incorrect. > > > > I think there are at least three options to prevent these problems. > > > > In the first option, we support only built-in aggregates which we know able > > to handle correctly. Supported aggregates can be identified using their > > OIDs. > > User-defined aggregates are not supported. I think this is the simplest and > > easiest way. > > I think this is enough for the first cut of IVM. So +1. If there is no objection, I will add the check of aggregate functions by this way. Thanks. Regards, Yugo Nagata -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Mon, 2 Dec 2019 13:48:40 -0300 Alvaro Herrera wrote: > On 2019-Dec-02, Yugo Nagata wrote: > > > On Mon, 02 Dec 2019 10:36:36 +0900 (JST) > > Tatsuo Ishii wrote: > > > > > >> One thing pending in this development line is how to catalogue > > > >> aggregate > > > >> functions that can be used in incrementally-maintainable views. > > > >> I saw a brief mention somewhere that the devels knew it needed to be > > > >> done, but I don't see in the thread that they got around to doing it. > > > >> Did you guys have any thoughts on how it can be represented in > > > >> catalogs? > > > >> It seems sine-qua-non ... > > > > > In the first option, we support only built-in aggregates which we know > > > > able > > > > to handle correctly. Supported aggregates can be identified using their > > > > OIDs. > > > > User-defined aggregates are not supported. I think this is the simplest > > > > and > > > > easiest way. > > > > > > I think this is enough for the first cut of IVM. So +1. > > > > If there is no objection, I will add the check of aggregate functions > > by this way. Thanks. > > The way I imagine things is that there's (one or more) new column in > pg_aggregate that links to the operator(s) (or function(s)?) that > support incremental update of the MV for that aggregate function. Is > that what you're proposing? The way I am proposing above is using OID to check if a aggregate can be used in IVM. This allows only a part of built-in aggreagete functions. This way you mentioned was proposed as one of options as following. On Fri, 29 Nov 2019 17:33:28 +0900 Yugo Nagata wrote: > Third, we can add a new attribute to pg_aggregate which shows if each > aggregate can be used in IVM. We don't need to use names or OIDs list of > supported aggregates although we need modification of the system catalogue. > > Regarding pg_aggregate, now we have aggcombinefn attribute for supporting > partial aggregation. Maybe we could use combine functions to calculate new > aggregate values in IVM when tuples are inserted into a table. However, in > the context of IVM, we also need other function used when tuples are deleted > from a table, so we can not use partial aggregation for IVM in the current > implementation. This might be another option to implement "inverse combine > function"(?) for IVM, but I am not sure it worth. If we add "inverse combine function" in pg_aggregate that takes two results of aggregating over tuples in a view and tuples in a delta, and produces a result of aggregating over tuples in the view after tuples in the delta are deleted from this, it would allow to calculate new aggregate values in IVM using aggcombinefn together when the aggregate function provides both functions. Another idea is to use support functions for moving-aggregate mode which are already provided in pg_aggregate. However, in this case, we have to apply tuples in the delta to the view one by one instead of applying after aggregating tuples in the delta. In both case, we can not use these support functions in SQL via SPI because the type of some aggregates is internal. We have to alter the current apply_delta implementation if we adopt a way using these support functions. Instead, we also can add support functions for IVM independent to partial aggregate or moving-aggregate. Maybe this is also one of options. Regards, Yugo Nagata -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Wed, 4 Dec 2019 21:18:02 +0900 nuko yokohama wrote: > Hi. > > I found the problem after running "ALTER MATERIALIZED VIEW ... RENAME TO". > If a view created with "CREATE INCREMENT MATERIALIZED VIEW" is renamed, > subsequent INSERT operations to the base table will fail. > > Error message. > ``` > ERROR: could not open relation with OID 0 Thank you for your pointing out this issue! This error occurs because the view's OID is retrieved using the view name. Considering that the name can be changed, this is obviously wrong. We'll fix it. Regards, Yugo Nagata > ``` > > Execution log. > ``` > [ec2-user@ip-10-0-1-10 ivm]$ psql -U postgres test -e -f > ~/test/ivm/alter_rename_bug.sql > DROP TABLE IF EXISTS table_x CASCADE; > psql:/home/ec2-user/test/ivm/alter_rename_bug.sql:1: NOTICE: drop cascades > to materialized view group_imv > DROP TABLE > CREATE TABLE table_x AS >SELECT generate_series(1, 1) AS id, >ROUND(random()::numeric * 100, 2) AS data, >CASE (random() * 5)::integer > WHEN 4 THEN 'group-a' > WHEN 3 THEN 'group-b' > ELSE 'group-c' >END AS part_key > ; > SELECT 1 >Table "public.table_x" > Column | Type | Collation | Nullable | Default > --+-+---+--+- > id | integer | | | > data | numeric | | | > part_key | text| | | > > DROP MATERIALIZED VIEW IF EXISTS group_imv; > psql:/home/ec2-user/test/ivm/alter_rename_bug.sql:15: NOTICE: materialized > view "group_imv" does not exist, skipping > DROP MATERIALIZED VIEW > CREATE INCREMENTAL MATERIALIZED VIEW group_imv AS > SELECT part_key, COUNT(*), MAX(data), MIN(data), SUM(data), AVG(data) > FROM table_x > GROUP BY part_key; > SELECT 3 > List of relations > Schema | Name| Type| Owner > +---+---+-- > public | group_imv | materialized view | postgres > public | table_x | table | postgres > (2 rows) > > Materialized view "public.group_imv" > Column | Type | Collation | Nullable | Default > ---+-+---+--+- > part_key | text| | | > count | bigint | | | > max | numeric | | | > min | numeric | | | > sum | numeric | | | > avg | numeric | | | > __ivm_count_max__ | bigint | | | > __ivm_count_min__ | bigint | | | > __ivm_count_sum__ | bigint | | | > __ivm_count_avg__ | bigint | | | > __ivm_sum_avg__ | numeric | | | > __ivm_count__ | bigint | | | > > SELECT * FROM group_imv ORDER BY part_key; > part_key | count | max | min |sum| avg > --+---+---+--+---+- > group-a | 1966 | 99.85 | 0.05 | 98634.93 | 50.1703611393692777 > group-b | 2021 | 99.99 | 0.17 | 102614.02 | 50.7738842157347848 > group-c | 6013 | 99.99 | 0.02 | 300968.43 | 50.0529569266589057 > (3 rows) > > ALTER MATERIALIZED VIEW group_imv RENAME TO group_imv2; > ALTER MATERIALIZED VIEW > List of relations > Schema |Name| Type| Owner > ++---+-- > public | group_imv2 | materialized view | postgres > public | table_x| table | postgres > (2 rows) > > Materialized view "public.group_imv2" > Column | Type | Collation | Nullable | Default > ---+-+---+--+- > part_key | text| | | > count | bigint | | | > max | numeric | | | > min | numeric | | | > sum | numeric | | | > avg | numeric | | | > __ivm_count_max__ | bigint | | | > __ivm_count_min__ | bigint | | | > __ivm_count_sum__ | bigint | | | > __ivm_count_avg__ | bigint | | | > __ivm_sum_avg__ | numeric | | | > __ivm_count__ | bigint | | | > > SET client_min_messages = debug5; > psql:/home/ec2-user/test/ivm/alter
Re: Implementing Incremental View Maintenance
Hi, Attached is the latest patch (v10) to add support for Incremental Materialized View Maintenance (IVM). IVM is a way to make materialized views up-to-date in which only incremental changes are computed and applied on views rather than recomputing the contents from scratch as REFRESH MATERIALIZED VIEW does. IVM can update materialized views more efficiently than recomputation when only small part of the view need updates. There are two approaches with regard to timing of view maintenance: immediate and deferred. In immediate maintenance, views are updated in the same transaction where its base table is modified. In deferred maintenance, views are updated after the transaction is committed, for example, when the view is accessed, as a response to user command like REFRESH, or periodically in background, and so on. This patch implements a kind of immediate maintenance, in which materialized views are updated immediately in AFTER triggers when a base table is modified. This supports views using: - inner and outer joins including self-join - some built-in aggregate functions (count, sum, agv, min, max) - a part of subqueries -- simple subqueries in FROM clause -- EXISTS subqueries in WHERE clause - DISTINCT and views with tuple duplicates === Here are major changes we made after the previous submitted patch: * Aggregate functions are checked if they can be used in IVM using their OID. Per comments from Alvaro Herrera. For this purpose, Gen_fmgrtab.pl was modified so that OIDs of aggregate functions are output to fmgroids.h. * Some bug fixes including: - Mistake of tab-completion of psql pointed out by nuko-san - A bug relating rename of matview pointed out by nuko-san - spelling errors - etc. * Add documentations for IVM * Patch is splited into eleven parts to make review easier as suggested by Amit Langote: - 0001: Add a new syntax: CREATE INCREMENTAL MATERIALIZED VIEW - 0002: Add a new column relisivm to pg_class - 0003: Change trigger.c to allow to prolong life span of tupestores containing Transition Tables generated via AFTER trigger - 0004: Add the basic IVM future using counting algorithm: This supports inner joins, DISTINCT, and tuple duplicates. - 0005: Change GEN_fmgrtab.pl to output aggregate function's OIDs - 0006: Add aggregates support for IVM - 0007: Add subqueries support for IVM - 0008: Add outer joins support for IVM - 0009: Add IVM support to psql command - 0010: Add regression tests for IVM - 0011: Add documentations for IVM === Todo: Currently, REFRESH and pg_dump/pg_restore is not supported, but we are working on them. Also, TRUNCATE is not supported. When TRUNCATE command is executed on a base table, nothing occurs on materialized views. We are now considering another better options, like: - Raise an error or warning when a base table is TRUNCATEed. - Make the view non-scannable (like WITH NO DATA) - Update the view in some ways. It would be easy for inner joins or aggregate views, but there is some difficult with outer joins. Regards, Yugo Nagata -- Yugo Nagata IVM_patches_v10.tar.gz Description: application/gzip
Re: Implementing Incremental View Maintenance
On Sun, 22 Dec 2019 20:54:41 +0900 nuko yokohama wrote: > SELECT statement that is not IMMUTABLE must not be specified when creating > a view. > > An expression SELECT statement that is not IMMUTABLE must not be specified > when creating a view. > > In the current implementation, a SELECT statement containing an expression > that is not IMMUTABLE can be specified when creating a view. > If an incremental materialized view is created from a SELECT statement that > contains an expression that is not IMMUTABLE, applying the SELECT statement > to the view returns incorrect results. > To prevent this, we propose that the same error occur when a non-IMMUTABLE > expression is specified in the "CREATE INDEX" statement. Thank you for pointing out this. That makes sense. The check of not-IMMUTABLE epressions is missing at creating IMMV. We'll add this. Thanks, Yugo Nagata > > The following is an inappropriate example. > > CREATE TABLE base (id int primary key, data text, ts timestamp); > CREATE TABLE > CREATE VIEW base_v AS SELECT * FROM base > WHERE ts >= (now() - '3 second'::interval); > CREATE VIEW > CREATE MATERIALIZED VIEW base_mv AS SELECT * FROM base > WHERE ts >= (now() - '3 second'::interval); > SELECT 0 > CREATE INCREMENTAL MATERIALIZED VIEW base_imv AS SELECT * FROM base > WHERE ts >= (now() - '3 second'::interval); > SELECT 0 > View "public.base_v" > Column |Type | Collation | Nullable | Default | > Storage | Description > +-+---+--+-+--+- > id | integer | | | | > plain| > data | text| | | | > extended | > ts | timestamp without time zone | | | | > plain| > View definition: > SELECT base.id, > base.data, > base.ts >FROM base > WHERE base.ts >= (now() - '00:00:03'::interval); > > Materialized view "public.base_mv" > Column |Type | Collation | Nullable | Default | > Storage | Stats target | Description > +-+---+--+-+--+--+- > id | integer | | | | > plain| | > data | text| | | | > extended | | > ts | timestamp without time zone | | | | > plain| | > View definition: > SELECT base.id, > base.data, > base.ts >FROM base > WHERE base.ts >= (now() - '00:00:03'::interval); > Access method: heap > > Materialized view "public.base_imv" > Column |Type | Collation | Nullable | > Default | Storage | Stats target | Description > ---+-+---+--+-+--+--+- > id| integer | | | > | plain| | > data | text| | | > | extended | | > ts| timestamp without time zone | | | > | plain| | > __ivm_count__ | bigint | | | > | plain| | > View definition: > SELECT base.id, > base.data, > base.ts >FROM base > WHERE base.ts >= (now() - '00:00:03'::interval); > Access method: heap > Incremental view maintenance: yes > > INSERT INTO base VALUES (generate_series(1,3), 'dummy', clock_timestamp()); > INSERT 0 3 > SELECT * FROM base_v ORDER BY id; > id | data | ts > +---+ > 1 | dummy | 2019-12-22 11:38:26.367481 > 2 | dummy | 2019-12-22 11:38:26.367599 > 3 | dummy | 2019-12-22 11:38:26.367606 > (3 rows) > > SELECT * FROM base_mv ORDER BY id; > id | data | ts > +--+ > (0 rows) > > REFRESH MATERIALIZED VIEW base_mv; > REFRESH MATERIALIZED VIEW > SELECT * FROM base_mv ORDER BY id; > id | data | ts > +---+ > 1 | dummy | 2019-12-22 11:38:26.367481 > 2 | dummy | 2019-12-22 11:38:26.367599 > 3 | dummy | 2019-12-22 11:38:26.367606 > (3 rows) > > SELECT * FROM
Re: Implementing Incremental View Maintenance
On Mon, 23 Dec 2019 02:26:09 + "tsunakawa.ta...@fujitsu.com" wrote: > From: legrand legrand > > For each insert into a base table there are 3 statements: > > - ANALYZE pg_temp_3.pg_temp_81976 > > - WITH updt AS ( UPDATE public.mv1 AS mv SET __ivm_count__ = ... > > - DROP TABLE pg_temp_3.pg_temp_81976 > > Does it also include CREATE TEMPORARY TABLE, because there's DROP? CREATE TEMPRARY TABLE is not called because temptables are created by make_new_heap() instead of queries via SPI. > I remember that repeated CREATE and DROP of temporary tables should be > avoided in PostgreSQL. Dropped temporary tables leave some unused memory in > CacheMemoryContext. If creation and deletion of temporary tables are done > per row in a single session, say loading of large amount of data, memory > bloat could crash the OS. That actually happened at a user's environment. > Plus, repeated create/drop may cause system catalog bloat as well even when > they are performed in different sessions. In a fortunate case, the garbage > records gather at the end of the system tables, and autovacuum will free > those empty areas by truncating data files. However, if some valid entry > persists after the long garbage area, the system tables would remain bloated. Thank you for explaining the problem. I understood that creating and dropping temprary tables is harmful more than I have thought. Although this is not a concrete plan, there are two ideas to reduce creating temporary tables: 1. Create a temporary table only once at the first view maintenance in this session. This is possible if we store names or oid of temporary tables used for each materialized view in memory. However, users may access to these temptables whenever during the session. 2. Use tuplestores instead of temprary tables. Tuplestores can be converted to Ephemeral Name Relation (ENR) and used in queries. It doesn't need updating system catalogs, but indexes can not be used to access. > > What kind of workload and data are you targeting with IVM? IVM (with immediate maintenance approach) would be efficient in situations where modifications on base tables are not frequent. In such situations, create and drop of temptalbes is not so frequent either, but it would be still possible that the problem you concern occurs. So, it seems worth to consider the way to reduce use of temptable. Regards, Yugo Nagata -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Mon, 23 Dec 2019 08:08:53 + "tsunakawa.ta...@fujitsu.com" wrote: > From: Yugo Nagata > > 1. Create a temporary table only once at the first view maintenance in > > this session. This is possible if we store names or oid of temporary > > tables used for each materialized view in memory. However, users may > > access to these temptables whenever during the session. > > > > 2. Use tuplestores instead of temprary tables. Tuplestores can be > > converted to Ephemeral Name Relation (ENR) and used in queries. > > It doesn't need updating system catalogs, but indexes can not be > > used to access. > > How about unlogged tables ? I thought the point of using a temp table is to > avoid WAL overhead. Hmm... this might be another option. However, if we use unlogged tables, we will need to create them in a special schema similar to pg_toast to split this from user tables. Otherwise, we need to create and drop unlogged tables repeatedly for each session. > > One concern about the temp table is that it precludes the use of distributed > transactions (PREPARE TRANSACTION fails if the transaction accessed a temp > table.) This could become a headache when FDW has supported 2PC (which > Sawada-san started and Horicuchi-san has taken over.) In the near future, > PostgreSQL may evolve into a shared nothing database with distributed > transactions like Postgres-XL. This makes sense since you mean that PREPARE TRANSACTION can not be used if any base table of incrementally maintainable materialized views is modified in the transaction, at least in the immediate maintenance. Maybe, this issue can be resolved if we implement the deferred maintenance planned in future because materialized views can be updated in other transactions in this way. > > > Regards > Takayuki Tsunakawa > > > -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Mon, 23 Dec 2019 03:41:18 -0700 (MST) legrand legrand wrote: > Hello, > regarding my initial post: > > > For each insert into a base table there are 3 statements: > > - ANALYZE pg_temp_3.pg_temp_81976 > > - WITH updt AS ( UPDATE public.mv1 AS mv SET __ivm_count__ = ... > > - DROP TABLE pg_temp_3.pg_temp_81976 > > For me there where 3 points to discuss: > - create/drop tables may bloat dictionnary tables > - create/drop tables prevents "WITH updt ..." from being shared (with some > plan caching) > - generates many lines in pg_stat_statements > > In fact I like the idea of a table created per session, but I would even > prefer a common "table" shared between all sessions like GLOBAL TEMPORARY > TABLE (or something similar) as described here: > https://www.postgresql.org/message-id/flat/157703426606.1198.2452090605041230054.pgcf%40coridan.postgresql.org#331e8344bbae904350af161fb43a0aa6 Although I have not looked into this thread, this may be help if this is implemented. However, it would be still necessary to truncate the table before the view maintenance because such tables always exist and can be accessed and modified by any users. -- Yugo Nagata
Re: Implementing Incremental View Maintenance
On Tue, 24 Dec 2019 07:07:35 + "tsunakawa.ta...@fujitsu.com" wrote: > From: Yugo Nagata > > On Mon, 23 Dec 2019 08:08:53 + > > "tsunakawa.ta...@fujitsu.com" wrote: > > > How about unlogged tables ? I thought the point of using a temp table is > > > to > > avoid WAL overhead. > > > > Hmm... this might be another option. However, if we use unlogged tables, > > we will need to create them in a special schema similar to pg_toast > > to split this from user tables. Otherwise, we need to create and drop > > unlogged tables repeatedly for each session. > > Maybe we can create the work tables in the same schema as the materialized > view, following: > > * Prefix the table name to indicate that the table is system-managed, thus > alluding to the user that manually deleting the table would break something. > This is like the system attribute __imv_count you are proposing. > > * Describe the above in the manual. Columns of serial and bigserial data > type similarly create sequences behind the scenes. > > * Make the work tables depend on the materialized view by recording the > dependency in pg_depend, so that Dropping the materialized view will also > drop its work tables. Maybe it works, but instead of using special names for work tables, we can also create a schema whose name is special and place work tables in this. This will not annoy users with information they are not interested in when, for example, psql meta-commands like \d are used. Anyway, I understood it is better to avoid creating and dropping temporary tables during view maintenance per statement. -- Yugo Nagata
Re: pgbnech: allow to cancel queries during benchmark
On Mon, 15 Jan 2024 16:49:44 +0900 (JST)
Tatsuo Ishii wrote:
> > On Wed, 6 Sep 2023 20:13:34 +0900
> > Yugo NAGATA wrote:
> >
> >> I attached the updated patch v3. The changes since the previous
> >> patch includes the following;
> >>
> >> I removed the unnecessary condition (&& false) that you
> >> pointed out in [1].
> >>
> >> The test was rewritten by using IPC::Run signal() and integrated
> >> to "001_pgbench_with_server.pl". This test is skipped on Windows
> >> because SIGINT causes to terminate the test itself as discussed
> >> in [2] about query cancellation test in psql.
> >>
> >> I added some comments to describe how query cancellation is
> >> handled as I explained in [1].
> >>
> >> Also, I found the previous patch didn't work on Windows so fixed it.
> >> On non-Windows system, a thread waiting a response of long query can
> >> be interrupted by SIGINT, but on Windows, threads do not return from
> >> waiting until queries they are running are cancelled. This is because,
> >> when the signal is received, the system just creates a new thread to
> >> execute the callback function specified by setup_cancel_handler, and
> >> other thread continue to run[3]. Therefore, the queries have to be
> >> cancelled in the callback function.
> >>
> >> [1]
> >> https://www.postgresql.org/message-id/a58388ac-5411-4760-ea46-71324d8324cb%40mines-paristech.fr
> >> [2]
> >> https://www.postgresql.org/message-id/20230906004524.2fd6ee049f8a6c6f2690b99c%40sraoss.co.jp
> >> [3] https://learn.microsoft.com/en-us/windows/console/handlerroutine
> >
> > I found that --disable-thread-safety option was removed in 68a4b58eca0329.
> > So, I removed codes involving ENABLE_THREAD_SAFETY from the patch.
> >
> > Also, I wrote a commit log draft.
>
> > Previously, Ctrl+C during benchmark killed pgbench immediately,
> > but queries running at that time were not cancelled.
>
> Better to mention explicitely that queries keep on running on the
> backend. What about this?
>
> Previously, Ctrl+C during benchmark killed pgbench immediately, but
> queries were not canceled and they keep on running on the backend
> until they tried to send the result to pgbench.
Thank you for your comments. I agree with you, so I fixed the message
as your suggestion.
> > The commit
> > fixes this so that cancel requests are sent for all connections
> > before pgbench exits.
>
> "sent for" -> "sent to"
Fixed.
> > Attached is the updated version, v4.
>
> +/* send cancel requests to all connections */
> +static void
> +cancel_all()
> +{
> + for (int i = 0; i < nclients; i++)
> + {
> + char errbuf[1];
> + if (client_states[i].cancel != NULL)
> + (void) PQcancel(client_states[i].cancel, errbuf,
> sizeof(errbuf));
> + }
> +}
> +
>
> Why in case of errors from PQCancel the error message is neglected? I
> think it's better to print out the error message in case of error.
Is the message useful for pgbench users? I saw the error is ignored
in pg_dump, for example in bin/pg_dump/parallel.c
/*
* Send QueryCancel to leader connection, if enabled. Ignore errors,
* there's not much we can do about them anyway.
*/
if (signal_info.myAH != NULL && signal_info.myAH->connCancel != NULL)
(void) PQcancel(signal_info.myAH->connCancel,
errbuf, sizeof(errbuf));
> + * On non-Windows, any callback function is not set. When SIGINT is
> + * received, CancelRequested is just set, and only thread #0 is
> interrupted
> + * and returns from waiting input from the backend. After that, the
> thread
> + * sends cancel requests to all benchmark queries.
>
> The second line is a little bit long according to the coding
> standard. Fix like this?
>
>* On non-Windows, any callback function is not set. When SIGINT is
> * received, CancelRequested is just set, and only thread #0 is
>* interrupted and returns from waiting input from the backend. After
>* that, the thread sends cancel requests to all benchmark queries.
Fixed.
The attached is the updated patch, v5.
Regards,
Yugo Nagata
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA
>From a42e6d47b27f2e91a02c5d934509070bd41bf79b Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 24 Ju
Re: Incremental View Maintenance, take 2
On Mon, 22 Jan 2024 13:51:08 +1100 Peter Smith wrote: > 2024-01 Commitfest. > > Hi, This patch has a CF status of "Needs Review" [1], but it seems > like there was some CFbot test failure last time it was run [2]. > Please have a look and post an updated version if necessary. Thank you for pointing out it. The CFbot failure is caused by a post [1] not by my patch-set, but regardless of it, I will heck if we need rebase and send the new version if necessary soon. [1] https://www.postgresql.org/message-id/CACJufxEoCCJE1vntJp1SWjen8vBUa3vZLgL%3DswPwar4zim976g%40mail.gmail.com Regards, Yugo Nagata > == > [1] https://commitfest.postgresql.org/46/4337/ > [2] https://cirrus-ci.com/task/6607979311529984 > > Kind Regards, > Peter Smith. -- Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
On Fri, 19 Jan 2024 17:46:03 +0900 (JST)
Tatsuo Ishii wrote:
> >> +/* send cancel requests to all connections */
> >> +static void
> >> +cancel_all()
> >> +{
> >> + for (int i = 0; i < nclients; i++)
> >> + {
> >> + char errbuf[1];
> >> + if (client_states[i].cancel != NULL)
> >> + (void) PQcancel(client_states[i].cancel, errbuf,
> >> sizeof(errbuf));
> >> + }
> >> +}
> >> +
> >>
> >> Why in case of errors from PQCancel the error message is neglected? I
> >> think it's better to print out the error message in case of error.
> >
> > Is the message useful for pgbench users? I saw the error is ignored
> > in pg_dump, for example in bin/pg_dump/parallel.c
>
> I think the situation is different from pg_dump. Unlike pg_dump, if
> PQcancel does not work, users can fix the problem by using
> pg_terminate_backend or kill command. In order to make this work, an
> appropriate error message is essential.
Makes sense. I fixed to emit an error message when PQcancel fails.
Also, I added some comments about the signal handling on Windows
to explain why the different way than non-Windows is required;
+* On Windows, a callback function is set in which query cancel requests
+* are sent to all benchmark queries running in the backend. This is
+* required because all threads running queries continue to run without
+* interrupted even when the signal is received.
+ *
Attached is the updated patch, v6.
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
>
>
--
Yugo NAGATA
>From 52579f3d31a2927d8818953fabf8a908466e4fcf Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 24 Jul 2023 21:53:28 +0900
Subject: [PATCH v6] Allow pgbnech to cancel queries during benchmark
Previously, Ctrl+C during benchmark killed pgbench immediately,
but queries were not cancelled nd they keep on running on the
backend until they tried to send the result to pgbench.
The commit fixes this so that cancel requests are sent to all
connections before pgbench exits.
In thread #0, setup_cancel_handler is called before the benchmark
so that CancelRequested is set when SIGINT is sent. When SIGINT
is sent during the benchmark, on non-Windows, thread #0 will be
interrupted, return from I/O wait, and send cancel requests to
all connections. After queries are cancelled, other threads also
be interrupted and pgbench will exit at the end. On Windows, cancel
requests are sent in the callback function specified by
setup_cancel_hander.
---
src/bin/pgbench/pgbench.c| 94
src/bin/pgbench/t/001_pgbench_with_server.pl | 42 +
2 files changed, 136 insertions(+)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 2574454839..e69c4af68a 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -596,6 +596,7 @@ typedef enum
typedef struct
{
PGconn *con; /* connection handle to DB */
+ PGcancel *cancel; /* query cancel */
int id;/* client No. */
ConnectionStateEnum state; /* state machine's current state. */
ConditionalStack cstack; /* enclosing conditionals state */
@@ -638,6 +639,8 @@ typedef struct
* here */
} CState;
+CState *client_states; /* status of all clients */
+
/*
* Thread state
*/
@@ -837,6 +840,10 @@ static void add_socket_to_set(socket_set *sa, int fd, int idx);
static int wait_on_socket_set(socket_set *sa, int64 usecs);
static bool socket_has_input(socket_set *sa, int fd, int idx);
+#ifdef WIN32
+static void pgbench_cancel_callback(void);
+#endif
+
/* callback used to build rows for COPY during data loading */
typedef void (*initRowMethod) (PQExpBufferData *sql, int64 curr);
@@ -3646,6 +3653,7 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
st->state = CSTATE_ABORTED;
break;
}
+ st->cancel = PQgetCancel(st->con);
/* reset now after connection */
now = pg_time_now();
@@ -4677,6 +4685,21 @@ disconnect_all(CState *state, int length)
finishCon(&state[i]);
}
+/* send cancel requests to all connections */
+static void
+cancel_all()
+{
+ for (int i = 0; i < nclients; i++)
+ {
+ char errbuf[256];
+ if (client_states[i].cancel != NULL)
+ {
+ if (!PQcancel(client_states[i].cancel, errbuf, sizeof(errbuf)))
+pg_log_error("Could not send cancel request: %s", errbuf);
+ }
+ }
+}
+
/*
* Remove old pgbench tables, if any exist
*/
@@ -7153,6 +7176,9 @@ main(int argc, char **argv)
}
}
+ /* enable threads to access the status of all clients */
+ client_states = state;
+
/* other CState initializations */
for (i = 0; i < nclients; i++)
{
@@ -7365,6 +7391,
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 2 Jan 2024 08:00:00 +0800
jian he wrote:
> On Mon, Nov 6, 2023 at 8:00 AM jian he wrote:
> >
> > minor doc issues.
> > Returns the chunk id of the TOASTed value, or NULL if the value is not
> > TOASTed.
> > Should it be "chunk_id"?
Thank you for your suggestion. As you pointed out, it is called "chunk_id"
in the documentation, so I rewrote it and also added a link to the section
where the TOAST table structure is explained.
> > you may place it after pg_create_logical_replication_slot entry to
> > make it look like alphabetical order.
I've been thinking about where we should place the function in the doc,
and I decided place it in the table of Database Object Size Functions
because I think pg_column_toast_chunk_id also would assist understanding
the result of size functions as similar to pg_column_compression; that is,
those function can explain why a large value in size could be stored in
a column.
> > There is no test. maybe we can add following to
> > src/test/regress/sql/misc.sql
> > create table val(t text);
> > INSERT into val(t) SELECT string_agg(
> > chr((ascii('B') + round(random() * 25)) :: integer),'')
> > FROM generate_series(1,2500);
> > select pg_column_toast_chunk_id(t) is not null from val;
> > drop table val;
Thank you for the test proposal. However, if we add a test, I want
to check that the chunk_id returned by the function exists in the
TOAST table, and that it returns NULL if the values is not TOASTed.
For the purpose, I wrote a test using a dynamic SQL since the table
name of the TOAST table have to be generated from the main table's OID.
> Hi
> the main C function (pg_column_toast_chunk_id) I didn't change.
> I added tests as mentioned above.
> tests put it on src/test/regress/sql/misc.sql, i hope that's fine.
> I placed pg_column_toast_chunk_id in "Table 9.99. Database Object
> Location Functions" (below Table 9.98. Database Object Size
> Functions).
I could not find any change in your patch from my previous patch.
Maybe, you attached wrong file. I attached a patch updated based
on your review, including the documentation fixes and a test.
What do you think about this it?
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 97af1b2300ecf07a34923da87a9d84e6aa963956 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v3] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17 ++
src/backend/utils/adt/varlena.c | 41 +
src/include/catalog/pg_proc.dat | 3 +++
3 files changed, 61 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 210c7c0b02..2d82331323 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->f
Rename setup_cancel_handler in pg_dump
Hi,
Attached is a simple patch to rename setup_cancel_handler()
in pg_dump/parallel.c.
I am proposing it because there is a public function with
the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
does not include fe_utils/cancel.h, so there is no conflict,
but I think it is better to use different names to reduce
possible confusion.
I guess there was no concerns when setup_cancel_handler in
pg_dump/parallel.c was introduced because the same name
function was not in fe_utils that could be used in common
between client tools.. The public setup_cancel_handler in
fe_utils was introduced in a4fd3aa719e, where this function
was moved from psql.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..261b23cb3f 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void pg_dump_setup_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that pg_dump_setup_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+pg_dump_setup_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ pg_dump_setup_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
Small fix on COPY ON_ERROR document
Hi, I found that the documentation of COPY ON_ERROR said COPY stops operation at the first error when "ON_ERROR is not specified.", but it also stop when ON_ERROR is specified to the default value, "stop". I attached a very small patch to fix this just for making the description more accurate. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 21a5c4a052..14c8ceab06 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -577,8 +577,8 @@ COPY count COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY +ON_ERROR is not specified or stop. +This should not lead to problems in the event of a COPY TO, but the target table will already have received earlier rows in a COPY FROM. These rows will not be visible or accessible, but they still occupy disk space. This might
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 13:59:09 +0900 Masahiko Sawada wrote: > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA wrote: > > > > Hi, > > > > I found that the documentation of COPY ON_ERROR said > > COPY stops operation at the first error when > > "ON_ERROR is not specified.", but it also stop when > > ON_ERROR is specified to the default value, "stop". > > > > I attached a very small patch to fix this just for > > making the description more accurate. > > Thank you for the patch! > > +1 to fix it. > > -ON_ERROR is not specified. This > -should not lead to problems in the event of a COPY > +ON_ERROR is not specified or stop. > +This should not lead to problems in the event of a COPY > > How about the followings for consistency with the description of the > ON_ERROR option? > > COPY stops operation at the first error if the stop value is specified > to the ON_ERROR option. This should not lead to ... Thank you for you review. However, after posting the previous patch, I noticed that I overlooked ON_ERROR works only for errors due to data type incompatibility (that is mentioned as "malformed data" in the ON_ERROR description, though). So, how about rewriting this like: COPY stops operation at the first error unless the error is due to data type incompatibility and a value other than stop is specified to the ON_ERROR option. I attached the updated patch. Regards, Yugo Nagata > > Regards, > > -- > Masahiko Sawada > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml index 21a5c4a052..3676bf0e87 100644 --- a/doc/src/sgml/ref/copy.sgml +++ b/doc/src/sgml/ref/copy.sgml @@ -576,9 +576,10 @@ COPY count -COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY +COPY stops operation at the first error unless the error +is due to data type incompatibility and a value other than +stop is specified to the ON_ERROR option. +This should not lead to problems in the event of a COPY TO, but the target table will already have received earlier rows in a COPY FROM. These rows will not be visible or accessible, but they still occupy disk space. This might
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 15:26:55 +0900 Masahiko Sawada wrote: > On Fri, Jan 26, 2024 at 2:40 PM Yugo NAGATA wrote: > > > > On Fri, 26 Jan 2024 13:59:09 +0900 > > Masahiko Sawada wrote: > > > > > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA wrote: > > > > > > > > Hi, > > > > > > > > I found that the documentation of COPY ON_ERROR said > > > > COPY stops operation at the first error when > > > > "ON_ERROR is not specified.", but it also stop when > > > > ON_ERROR is specified to the default value, "stop". > > > > > > > > I attached a very small patch to fix this just for > > > > making the description more accurate. > > > > > > Thank you for the patch! > > > > > > +1 to fix it. > > > > > > -ON_ERROR is not specified. This > > > -should not lead to problems in the event of a COPY > > > +ON_ERROR is not specified or > > > stop. > > > +This should not lead to problems in the event of a COPY > > > > > > How about the followings for consistency with the description of the > > > ON_ERROR option? > > > > > > COPY stops operation at the first error if the stop value is specified > > > to the ON_ERROR option. This should not lead to ... > > > > Thank you for you review. However, after posting the previous patch, > > I noticed that I overlooked ON_ERROR works only for errors due to data > > type incompatibility (that is mentioned as "malformed data" in the > > ON_ERROR description, though). > > Right. > > > > > So, how about rewriting this like: > > > > COPY stops operation at the first error unless the error is due to data > > type incompatibility and a value other than stop is specified to the > > ON_ERROR option. > > Hmm, this sentence seems not very readable to me, especially the "COPY > stops ... unless ... a value other than stop is specified ..." part. I > think we can simplify it: I can agree with your opinion on the readability, but... > COPY stops operation at the first data type incompatibility error if > the stop value is specified to the ON_ERROR option. This statement doesn't explain COPY also stops when an error other than data type incompatibility (e.g. constrain violations) occurs. Maybe, we can separate the sentese to two, for example: COPY stops operation at the first error. (The exception is if the error is due to data type incompatibility and a value other than stop is specified to the ON_ERROR option.) Regards, Yugo Nagata > Regards, > > -- > Masahiko Sawada > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Thu, 25 Jan 2024 23:33:22 -0700
"David G. Johnston" wrote:
> On Thu, Jan 25, 2024 at 10:40 PM Yugo NAGATA wrote:
>
> > On Fri, 26 Jan 2024 13:59:09 +0900
> > Masahiko Sawada wrote:
> >
> > > On Fri, Jan 26, 2024 at 11:28 AM Yugo NAGATA
> > wrote:
> > > >
> > > > Hi,
> > > >
> > > > I found that the documentation of COPY ON_ERROR said
> > > > COPY stops operation at the first error when
> > > > "ON_ERROR is not specified.", but it also stop when
> > > > ON_ERROR is specified to the default value, "stop".
> > > >
> >
> >
> I'm finding the current wording surrounding ON_ERROR to not fit in with the
> rest of the page. I've tried to improve things by being a bit more
> invasive.
Introducing the ON_ERROR option changed the behavior of COPY about error
handling to some extent, so it might be good to rewrite a bit more parts
of the documentation as you suggest.
> This first hunk updates the description of the COPY command to include
> describing the purpose of the ON_ERROR clause.
>
> I too am concerned about the word "parsed" here, and "malformed" in the
> more detailed descriptions; this would need to be modified to better
> reflect the circumstances under which ignore happens.
I think "errors due to data type incompatibility" would be nice to describe
the errors to be ignored by ON_ERROR, as used in the NOTICE message output.
> diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
> index 21a5c4a052..5fe4c9f747 100644
> --- a/doc/src/sgml/ref/copy.sgml
> +++ b/doc/src/sgml/ref/copy.sgml
> @@ -90,6 +90,14 @@ COPY { class="parameter">table_name [ ( in the pg_stat_progress_copy view. See
> for details.
>
> +
> +
> +By default, COPY will abort if it encounters errors.
> +For non-binary format COPY FROM the user can specify
> +to instead ignore input rows that cannot be parsed by specifying
> +the ignore option to the ON_ERROR
> +clause.
> +
>
>
>
> The following was, IMO, too much text about something not to worry about,
> COPY TO and ignore mode. The point is dead tuples on error and running
> vacuum. It doesn't really even matter what caused the error, though if the
> error was even before rows started to be imported then obviously there
> would be no dead tuples.
>
> Oh, and the word "first" really isn't adding anything here that we cannot
> reasonably leave to common sense, IMO. We either ignore all errors or stop
> on the first one. There isn't a stop mode that is capable of bypassing
> errors and the ignore mode doesn't have a "first n" option so one assumes
> all errors are ignored.
>
> @@ -576,15 +583,12 @@ COPY class="parameter">count
>
>
>
> -COPY stops operation at the first error when
> -ON_ERROR is not specified. This
> -should not lead to problems in the event of a COPY
> -TO, but the target table will already have received
> -earlier rows in a COPY FROM. These rows will not
> -be visible or accessible, but they still occupy disk space. This might
> -amount to a considerable amount of wasted disk space if the failure
> -happened well into a large copy operation. You might wish to invoke
> -VACUUM to recover the wasted space.
> +A failed COPY FROM command will have performed
> +physical insertion of all rows prior to the malformed one.
As you said that it does not matter what caused the error, I don't think
we have to use "malformed" here. Instead, we could say "we will have
performed physical insertion of rows before the failure."
> +While these rows will not be visible or accessible, they still occupy
> +disk space. This might amount to a considerable amount of wasted disk
> +space if the failure happened well into a large copy operation.
> +VACUUM should be used to recover the wasted space.
>
>
>
>
> David J.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 00:00:57 -0700 "David G. Johnston" wrote: > On Thursday, January 25, 2024, Yugo NAGATA wrote: > > > > > Maybe, we can separate the sentese to two, for example: > > > > COPY stops operation at the first error. (The exception is if the error > > is due to data type incompatibility and a value other than stop is > > specified > > to the ON_ERROR option.) > > > > I’d lean more toward saying: > > Copy from can be instructed to ignore errors that arise from casting input > data to the data types of the target columns by setting the on_error option > to ignore. Skipping the entire input row in the process. > > The last part is because another way to ignore would be to set null values > for those columns. That makes sense. Is is a bit ambiguous to just say "skips malformed data"; it might be unclear for users if the data in the problematic column is skipped (NULL is set) or the entire row is skipped. Also, setting null values for those columns could be a future feature of ON_ERROR option. > > That a command stops on an error is the assumed behavior throughout the > system, writing “copy stops operation at the first error.” just seems > really unnecessary. I think we need to describe this default behavior explicitly somewhere, as you suggested in the other post [1]. [1] https://www.postgresql.org/message-id/CAKFQuwZJZ6uaS2B7qpL2FJzWBsnDdzgtbsW3pH9zuT6vC3fH%2Bg%40mail.gmail.com Regards, Yugo Nagata > I will need to make this tweak and probably a couple others to my own > suggestions in 12 hours or so. > > David J. -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Fri, 26 Jan 2024 08:04:45 -0700 "David G. Johnston" wrote: > On Fri, Jan 26, 2024 at 2:30 AM Yugo NAGATA wrote: > > > On Fri, 26 Jan 2024 00:00:57 -0700 > > "David G. Johnston" wrote: > > > > > I will need to make this tweak and probably a couple others to my own > > > suggestions in 12 hours or so. > > > > > > > > And here is my v2. > > Notably I choose to introduce the verbiage "soft error" and then define in > the ON_ERROR clause the specific soft error that matters here - "invalid > input syntax". I am not sure we should use "soft error" without any explanation because it seems to me that the meaning of words is unclear for users. This is explained in src/backend/utils/fmgr/README; * Some callers of datatype input functions (and in future perhaps other classes of functions) pass an instance of ErrorSaveContext. This indicates that the caller wishes to handle "soft" errors without a transaction-terminating exception being thrown: instead, the callee should store information about the error cause in the ErrorSaveContext struct and return a dummy result value. However, this is not mentioned in the documentation anywhere. So, it would be hard for users to understand what "soft error" is because they would not read README. Also, I think "invalid input syntax" is a bit ambiguous. For example, COPY FROM raises an error when the number of input column does not match to the table schema, but this error is not ignored by ON_ERROR while this seems to fall into the category of "invalid input syntax". I found the description as followings in the documentation in COPY: The column values themselves are strings (...) acceptable to the input function, of each attribute's data type. So, keeping consistency with the existing description, we can say: "Specifies which how to behave when encountering an error due to column values unacceptable to the input function of each attribute's data type." Currently, ON_ERROR doesn't support other soft errors, so it can explain it more simply without introducing the new concept, "soft error" to users. > I also note the log message behavior when ignore mode is chosen. I haven't > confirmed that it is accurate but that is readily tweaked if approved of. > + An INFO level context message containing the ignored row count is + emitted at the end of the COPY FROM if at least one row was discarded. The log level is NOTICE not INFO. -COPY stops operation at the first error when -ON_ERROR is not specified. This -should not lead to problems in the event of a COPY -TO, but the target table will already have received -earlier rows in a COPY FROM. These rows will not -be visible or accessible, but they still occupy disk space. This might +The COPY FROM command physically inserts input rows +into the table as it progresses. If the command fails these rows are +left in a deleted state, still occupying disk space. This might amount to a considerable amount of wasted disk space if the failure -happened well into a large copy operation. You might wish to invoke -VACUUM to recover the wasted space. +happened well into a large copy operation. VACUUM +should be used to recover the wasted space. I think "left in a deleted state" is also unclear for users because this explains the internal state but not how looks from user's view.How about leaving the explanation "These rows will not be visible or accessible" in the existing statement? Regards, Yugo Nagata -- Yugo NAGATA
Re: Small fix on COPY ON_ERROR document
On Sun, 28 Jan 2024 19:14:58 -0700 "David G. Johnston" wrote: > > Also, I think "invalid input syntax" is a bit ambiguous. For example, > > COPY FROM raises an error when the number of input column does not match > > to the table schema, but this error is not ignored by ON_ERROR while > > this seems to fall into the category of "invalid input syntax". > > > > It is literally the error text that appears if one were not to ignore it. > It isn’t a category of errors. But I’m open to ideas here. But being > explicit with what on actually sees in the system seemed preferable to > inventing new classification terms not otherwise used. Thank you for explanation! I understood the words was from the error messages that users actually see. However, as Torikoshi-san said in [1], errors other than valid input syntax (e.g. range error) can be also ignored, therefore it would be better to describe to be ignored errors more specifically. [1] https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com > > > > > So, keeping consistency with the existing description, we can say: > > > > "Specifies which how to behave when encountering an error due to > > column values unacceptable to the input function of each attribute's > > data type." > > > Yeah, I was considering something along those lines as an option as well. > But I’d rather add that wording to the glossary. Although I am still be not convinced if we have to introduce the words "soft error" to the documentation, I don't care it if there are no other opposite opinions. > > > Currently, ON_ERROR doesn't support other soft errors, so it can explain > > it more simply without introducing the new concept, "soft error" to users. > > > > > Good point. Seems we should define what user-facing errors are ignored > anywhere in the system and if we aren’t consistently leveraging these in > all areas/commands make the necessary qualifications in those specific > places. > > > I think "left in a deleted state" is also unclear for users because this > > explains the internal state but not how looks from user's view.How about > > leaving the explanation "These rows will not be visible or accessible" in > > the existing statement? > > > > Just visible then, I don’t like an “or” there and as tuples at least they > are accessible to the system, in vacuum especially. But I expected the > user to understand “as if you deleted it” as their operational concept more > readily than visible. I think this will be read by people who haven’t read > MVCC to fully understand what visible means but know enough to run vacuum > to clean up updated and deleted data as a rule. Ok, I agree we can omit "or accessible". How do you like the followings? Still redundant? "If the command fails, these rows are left in a deleted state; these rows will not be visible, but they still occupy disk space. " Regards, Yugo Nagata -- Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Fri, 26 Jan 2024 08:08:29 -0700 "David G. Johnston" wrote: > Hi, > > The option choice of "ignore" in the COPY ON_ERROR clause seems overly > generic. There would seem to be two relevant ways to ignore bad column > input data - drop the entire row or just set the column value to null. I > can see us wanting to provide the set to null option and in any case having > the option name be explicit that it ignores the row seems like a good idea. I am not in favour of renaming the option name "ignore", instead we can use another style of name for the option to set the column value to NULL, for example, "set_to_null". (Maybe, we can make a more generic option "set_to (col, val)" that can set the value of column specified by "col" value to the specified value "val" (e.g. 'N/A') on a soft error, although the syntax would be a bit complex...) IMO, it is more simple to define "ignore" as to skip the entire row rather than having variety of "ignore". Once defined it so, the option to set the column value to NULL should not be called "ignore" because values in other columns will be inserted. Regards, Yugo Nagata > > David J. -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 12:12:31 +0800
jian he wrote:
> On Fri, Jan 26, 2024 at 8:42 AM Yugo NAGATA wrote:
> >
> > On Tue, 2 Jan 2024 08:00:00 +0800
> > jian he wrote:
> >
> > > On Mon, Nov 6, 2023 at 8:00 AM jian he
> > > wrote:
> > > >
> > > > minor doc issues.
> > > > Returns the chunk id of the TOASTed value, or NULL if the value is not
> > > > TOASTed.
> > > > Should it be "chunk_id"?
> >
> > Thank you for your suggestion. As you pointed out, it is called "chunk_id"
> > in the documentation, so I rewrote it and also added a link to the section
> > where the TOAST table structure is explained.
> >
> > > > you may place it after pg_create_logical_replication_slot entry to
> > > > make it look like alphabetical order.
> >
> > I've been thinking about where we should place the function in the doc,
> > and I decided place it in the table of Database Object Size Functions
> > because I think pg_column_toast_chunk_id also would assist understanding
> > the result of size functions as similar to pg_column_compression; that is,
> > those function can explain why a large value in size could be stored in
> > a column.
>
> diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
> index 210c7c0b02..2d82331323 100644
> --- a/doc/src/sgml/func.sgml
> +++ b/doc/src/sgml/func.sgml
> @@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn +
> pd.segment_number * ps.setting::int + :offset
>
>
>
> +
> +
> +
> + pg_column_toast_chunk_id
> +
> +pg_column_toast_chunk_id ( "any" )
> +oid
> +
> +
> +Shows the chunk_id of an on-disk
> +TOASTed value. Returns NULL
> +if the value is un-TOASTed or not on-disk.
> +See for details about
> +TOAST.
> +
> +
>
> v3 patch will place it on `Table 9.97. Replication Management Functions`
> I agree with you. it should be placed after pg_column_compression. but
> apply your patch, it will be at
>
>
> > > > There is no test. maybe we can add following to
> > > > src/test/regress/sql/misc.sql
> > > > create table val(t text);
> > > > INSERT into val(t) SELECT string_agg(
> > > > chr((ascii('B') + round(random() * 25)) :: integer),'')
> > > > FROM generate_series(1,2500);
> > > > select pg_column_toast_chunk_id(t) is not null from val;
> > > > drop table val;
> >
> > Thank you for the test proposal. However, if we add a test, I want
> > to check that the chunk_id returned by the function exists in the
> > TOAST table, and that it returns NULL if the values is not TOASTed.
> > For the purpose, I wrote a test using a dynamic SQL since the table
> > name of the TOAST table have to be generated from the main table's OID.
> >
> > > Hi
> > > the main C function (pg_column_toast_chunk_id) I didn't change.
> > > I added tests as mentioned above.
> > > tests put it on src/test/regress/sql/misc.sql, i hope that's fine.
> > > I placed pg_column_toast_chunk_id in "Table 9.99. Database Object
> > > Location Functions" (below Table 9.98. Database Object Size
> > > Functions).
> >
> > I could not find any change in your patch from my previous patch.
> > Maybe, you attached wrong file. I attached a patch updated based
> > on your review, including the documentation fixes and a test.
> > What do you think about this it?
> >
>
> sorry, I had attached the wrong file.
> but your v3 also has no tests, documentation didn't fix.
> maybe you also attached the wrong file too?
>
Sorry, I also attached a wrong file.
Attached is the correct one.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 97af1b2300ecf07a34923da87a9d84e6aa963956 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v3] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17 ++
src/backend/utils/adt/varlena.c | 41 +
src/include/catalog/pg_proc.dat | 3 +++
3 files changed, 61 insertions(+)
diff --git
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 13:47:45 +0800
jian he wrote:
> On Tue, Jan 30, 2024 at 12:35 PM Yugo NAGATA wrote:
> >
> >
> > Sorry, I also attached a wrong file.
> > Attached is the correct one.
> I think you attached the wrong file again. also please name it as v4.
Opps..sorry, again.
I attached the correct one, v4.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 8c0f9993c49d1d4ed1bb3e10ba26652da98cd41e Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v4] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 18 +
src/test/regress/sql/misc_functions.sql | 18 +
5 files changed, 97 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 210c7c0b02..2d82331323 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28078,6 +28078,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 58811a6530..1d4521ac1f 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7454,6 +7454,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value',
+ proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype => 'oid',
+ proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
{ oid => '2322',
descr => 'total disk space usage for the specified tablespace',
proname => 'pg_tablespace_size', provolatile => 'v', prorettype => 'int8',
diff --git a/src/test/regress/expected/misc_functions.out b/src/test/regress/expected/misc_functions.out
index 9302134077..6a1fcc22f5 100644
--- a/src/test/regress/expected/misc_functions.out
+++ b/src/test/regress/expected/misc_functions.out
@@ -670,3 +670,21 @@ FROM pg_walfile_name_offset('0/0'::pg_lsn + :segment_size - 1),
0 | t
(1 row)
+-- Test pg_column_toast_chunk_id:
+-- Check if the returned chunk_id exists in the TOAST ta
Re: Rename setup_cancel_handler in pg_dump
On Tue, 30 Jan 2024 13:44:28 +0100
Daniel Gustafsson wrote:
> > On 26 Jan 2024, at 01:42, Yugo NAGATA wrote:
>
> > I am proposing it because there is a public function with
> > the same name in fe_utils/cancel.c. I know pg_dump/parallel.c
> > does not include fe_utils/cancel.h, so there is no conflict,
> > but I think it is better to use different names to reduce
> > possible confusion.
>
> Given that a "git grep setup_cancel_hander" returns hits in pg_dump along with
> other frontend utils, I can see the risk of confusion.
Thank you for looking into it!
> -setup_cancel_handler(void)
> +pg_dump_setup_cancel_handler(void)
>
> We don't have any other functions prefixed with pg_dump_, based on the naming
> of the surrounding code in the file I wonder if set_cancel_handler is a more
> appropriate name?
Agreed. Here is a updated patch.
Regards,
Yugo Nagata
>
> --
> Daniel Gustafsson
>
--
Yugo NAGATA
diff --git a/src/bin/pg_dump/parallel.c b/src/bin/pg_dump/parallel.c
index 188186829c..a09247fae4 100644
--- a/src/bin/pg_dump/parallel.c
+++ b/src/bin/pg_dump/parallel.c
@@ -204,7 +204,7 @@ static ParallelSlot *GetMyPSlot(ParallelState *pstate);
static void archive_close_connection(int code, void *arg);
static void ShutdownWorkersHard(ParallelState *pstate);
static void WaitForTerminatingWorkers(ParallelState *pstate);
-static void setup_cancel_handler(void);
+static void set_cancel_handler(void);
static void set_cancel_pstate(ParallelState *pstate);
static void set_cancel_slot_archive(ParallelSlot *slot, ArchiveHandle *AH);
static void RunWorker(ArchiveHandle *AH, ParallelSlot *slot);
@@ -550,7 +550,7 @@ sigTermHandler(SIGNAL_ARGS)
/*
* Some platforms allow delivery of new signals to interrupt an active
* signal handler. That could muck up our attempt to send PQcancel, so
- * disable the signals that setup_cancel_handler enabled.
+ * disable the signals that set_cancel_handler enabled.
*/
pqsignal(SIGINT, SIG_IGN);
pqsignal(SIGTERM, SIG_IGN);
@@ -605,7 +605,7 @@ sigTermHandler(SIGNAL_ARGS)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
/*
* When forking, signal_info.handler_set will propagate into the new
@@ -705,7 +705,7 @@ consoleHandler(DWORD dwCtrlType)
* Enable cancel interrupt handler, if not already done.
*/
static void
-setup_cancel_handler(void)
+set_cancel_handler(void)
{
if (!signal_info.handler_set)
{
@@ -737,7 +737,7 @@ set_archive_cancel_info(ArchiveHandle *AH, PGconn *conn)
* important that this happen at least once before we fork off any
* threads.
*/
- setup_cancel_handler();
+ set_cancel_handler();
/*
* On Unix, we assume that storing a pointer value is atomic with respect
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 30 Jan 2024 14:57:20 +0800
jian he wrote:
> On Tue, Jan 30, 2024 at 1:56 PM Yugo NAGATA wrote:
> >
> > I attached the correct one, v4.
> >
>
> +-- Test pg_column_toast_chunk_id:
> +-- Check if the returned chunk_id exists in the TOAST table
> +CREATE TABLE test_chunk_id (v1 text, v2 text);
> +INSERT INTO test_chunk_id VALUES (
> + repeat('0123456789', 10), -- v1: small enough not to be TOASTed
> + repeat('0123456789', 10)); -- v2: large enough to be TOASTed
>
> select pg_size_pretty(10::bigint);
> return 98kb.
>
> I think this is just too much, maybe I didn't consider the
> implications of compression.
> Anyway, I refactored the tests, making the toast value size be small.
Actually the data is compressed and the size is much smaller,
but I agree with you it is better not to generate large data unnecessarily.
I rewrote the test to disallow compression in the toast data using
"ALTER TABLE ... SET STORAGE EXTERNAL". In this case, any text larger
than 2k will be TOASTed on disk without compression, and it makes the
test simple, not required to use string_agg.
>
> I aslo refactor the doc.
> pg_column_toast_chunk_id entry will be right after pg_column_compression
> entry.
> You can check the screenshot.
I found the document order was not changed between my patch and yours.
In both, pg_column_toast_chunk_id entry is right after
pg_column_compression.
Here is a updated patch, v6.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From dfec0fdb52c6aad8846b5c984e111dc8b2985e1a Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v6] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 6788ba8ef4..4116aaff7a 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28502,6 +28502,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 29af4ce65d..9ab71646e3 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7454,6 +7454,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum'
Re: Small fix on COPY ON_ERROR document
On Mon, 29 Jan 2024 15:47:25 +0900
Yugo NAGATA wrote:
> On Sun, 28 Jan 2024 19:14:58 -0700
> "David G. Johnston" wrote:
>
> > > Also, I think "invalid input syntax" is a bit ambiguous. For example,
> > > COPY FROM raises an error when the number of input column does not match
> > > to the table schema, but this error is not ignored by ON_ERROR while
> > > this seems to fall into the category of "invalid input syntax".
> >
> >
> >
> > It is literally the error text that appears if one were not to ignore it.
> > It isn’t a category of errors. But I’m open to ideas here. But being
> > explicit with what on actually sees in the system seemed preferable to
> > inventing new classification terms not otherwise used.
>
> Thank you for explanation! I understood the words was from the error messages
> that users actually see. However, as Torikoshi-san said in [1], errors other
> than valid input syntax (e.g. range error) can be also ignored, therefore it
> would be better to describe to be ignored errors more specifically.
>
> [1]
> https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com
>
> >
> > >
> > > So, keeping consistency with the existing description, we can say:
> > >
> > > "Specifies which how to behave when encountering an error due to
> > > column values unacceptable to the input function of each attribute's
> > > data type."
> >
> >
> > Yeah, I was considering something along those lines as an option as well.
> > But I’d rather add that wording to the glossary.
>
> Although I am still be not convinced if we have to introduce the words
> "soft error" to the documentation, I don't care it if there are no other
> opposite opinions.
Attached is a updated patch v3, which is a version that uses the above
wording instead of "soft error".
> >
> > > Currently, ON_ERROR doesn't support other soft errors, so it can explain
> > > it more simply without introducing the new concept, "soft error" to users.
> > >
> > >
> > Good point. Seems we should define what user-facing errors are ignored
> > anywhere in the system and if we aren’t consistently leveraging these in
> > all areas/commands make the necessary qualifications in those specific
> > places.
> >
>
> > > I think "left in a deleted state" is also unclear for users because this
> > > explains the internal state but not how looks from user's view.How about
> > > leaving the explanation "These rows will not be visible or accessible" in
> > > the existing statement?
> > >
> >
> > Just visible then, I don’t like an “or” there and as tuples at least they
> > are accessible to the system, in vacuum especially. But I expected the
> > user to understand “as if you deleted it” as their operational concept more
> > readily than visible. I think this will be read by people who haven’t read
> > MVCC to fully understand what visible means but know enough to run vacuum
> > to clean up updated and deleted data as a rule.
>
> Ok, I agree we can omit "or accessible". How do you like the followings?
> Still redundant?
>
> "If the command fails, these rows are left in a deleted state;
> these rows will not be visible, but they still occupy disk space. "
Also, the above statement is used in the patch.
Regards,
Yugo Nagata
> Regards,
> Yugo Nagata
>
> --
> Yugo NAGATA
>
>
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/copy.sgml b/doc/src/sgml/ref/copy.sgml
index 21a5c4a052..10cfc3f0ad 100644
--- a/doc/src/sgml/ref/copy.sgml
+++ b/doc/src/sgml/ref/copy.sgml
@@ -90,6 +90,13 @@ COPY { table_name [ ( pg_stat_progress_copy view. See
for details.
+
+
+By default, COPY will fail if it encounters an error
+during processing. For use cases where a best-effort attempt at loading
+the entire file is desired, the ON_ERROR clause can
+be used to specify some other behavior.
+
@@ -378,17 +385,20 @@ COPY { table_name [ ( ON_ERROR
- Specifies which
- error_action to perform when there is malformed data in the input.
- Currently, only stop (default) and ignore
- values are supported.
- If the stop value is specified,
- COPY stops operation at the first error.
- If the ignore value is specified,
- COPY skips malformed data and continues copying data.
- The option is allowed only in COPY FROM.
- Only stop value is allowed when
- using binary format.
+ Specifies which how to beha
Re: Small fix on COPY ON_ERROR document
On Fri, 02 Feb 2024 11:29:41 +0900 torikoshia wrote: > On 2024-02-01 15:16, Yugo NAGATA wrote: > > On Mon, 29 Jan 2024 15:47:25 +0900 > > Yugo NAGATA wrote: > > > >> On Sun, 28 Jan 2024 19:14:58 -0700 > >> "David G. Johnston" wrote: > >> > >> > > Also, I think "invalid input syntax" is a bit ambiguous. For example, > >> > > COPY FROM raises an error when the number of input column does not > >> > > match > >> > > to the table schema, but this error is not ignored by ON_ERROR while > >> > > this seems to fall into the category of "invalid input syntax". > >> > > >> > > >> > > >> > It is literally the error text that appears if one were not to ignore it. > >> > It isn’t a category of errors. But I’m open to ideas here. But being > >> > explicit with what on actually sees in the system seemed preferable to > >> > inventing new classification terms not otherwise used. > >> > >> Thank you for explanation! I understood the words was from the error > >> messages > >> that users actually see. However, as Torikoshi-san said in [1], errors > >> other > >> than valid input syntax (e.g. range error) can be also ignored, > >> therefore it > >> would be better to describe to be ignored errors more specifically. > >> > >> [1] > >> https://www.postgresql.org/message-id/7f1457497fa3bf9dfe486f162d1c8ec6%40oss.nttdata.com > >> > >> > > >> > > > >> > > So, keeping consistency with the existing description, we can say: > >> > > > >> > > "Specifies which how to behave when encountering an error due to > >> > > column values unacceptable to the input function of each attribute's > >> > > data type." > >> > > >> > > >> > Yeah, I was considering something along those lines as an option as well. > >> > But I’d rather add that wording to the glossary. > >> > >> Although I am still be not convinced if we have to introduce the words > >> "soft error" to the documentation, I don't care it if there are no > >> other > >> opposite opinions. > > > > Attached is a updated patch v3, which is a version that uses the above > > wording instead of "soft error". > > > >> > > >> > > Currently, ON_ERROR doesn't support other soft errors, so it can > >> > > explain > >> > > it more simply without introducing the new concept, "soft error" to > >> > > users. > >> > > > >> > > > >> > Good point. Seems we should define what user-facing errors are ignored > >> > anywhere in the system and if we aren’t consistently leveraging these in > >> > all areas/commands make the necessary qualifications in those specific > >> > places. > >> > > >> > >> > > I think "left in a deleted state" is also unclear for users because > >> > > this > >> > > explains the internal state but not how looks from user's view.How > >> > > about > >> > > leaving the explanation "These rows will not be visible or accessible" > >> > > in > >> > > the existing statement? > >> > > > >> > > >> > Just visible then, I don’t like an “or” there and as tuples at least they > >> > are accessible to the system, in vacuum especially. But I expected the > >> > user to understand “as if you deleted it” as their operational concept > >> > more > >> > readily than visible. I think this will be read by people who haven’t > >> > read > >> > MVCC to fully understand what visible means but know enough to run vacuum > >> > to clean up updated and deleted data as a rule. > >> > >> Ok, I agree we can omit "or accessible". How do you like the > >> followings? > >> Still redundant? > >> > >> "If the command fails, these rows are left in a deleted state; > >> these rows will not be visible, but they still occupy disk space. " > > > > Also, the above statement is used in the patch. > > Thanks for updating the patch! > > I like your description which doesn't use the word soft error. Thank you for your comments! > > Here are m
Re: Checking MINIMUM_VERSION_FOR_WAL_SUMMARIES
On Fri, 2 Feb 2024 01:11:27 +0100
Artur Zakirov wrote:
> Hi hackers,
>
> during reading the source code of new incremental backup functionality
> I noticed that the following condition can by unintentional:
>
> /*
> * For newer server versions, likewise create pg_wal/summaries
> */
> if (PQserverVersion(conn) < MINIMUM_VERSION_FOR_WAL_SUMMARIES)
> {
> ...
>
> if (pg_mkdir_p(summarydir, pg_dir_create_mode) != 0 &&
> errno != EEXIST)
> pg_fatal("could not create directory \"%s\": %m", summarydir);
> }
>
> This is from src/bin/pg_basebackup/pg_basebackup.c.
>
> Is the condition correct? Shouldn't it be ">=". Otherwise the function
> will create "/summaries" only for older PostgreSQL versions.
>
> I've attached a patch to fix it in case this is a typo.
I also think it should be ">="
Also, if so, I don't think the check of MINIMUM_VERSION_FOR_PG_WAL
in the block is required, because the directory name is always pg_wal
in the new versions.
Regards,
Yugo Nagata
>
> --
> Artur
--
Yugo NAGATA
Re: InstallXLogFileSegment() vs concurrent WAL flush
On Fri, 2 Feb 2024 11:18:18 +0100
Thomas Munro wrote:
> Hi,
>
> New WAL space is created by renaming a file into place. Either a
> newly created file with a temporary name or, ideally, a recyclable old
> file with a name derived from an old LSN. I think there is a data
> loss window between rename() and fsync(parent_directory). A
> concurrent backend might open(new_name), write(), fdatasync(), and
> then we might lose power before the rename hits the disk. The data
> itself would survive the crash, but recovery wouldn't be able to find
> and replay it. That might break the log-before-data rule or forget a
> transaction that has been reported as committed to a client.
>
> Actual breakage would presumably require really bad luck, and I
> haven't seen this happen or anything, it just occurred to me while
> reading code, and I can't see any existing defences.
>
> One simple way to address that would be to make XLogFileInitInternal()
> wait for InstallXLogFileSegment() to finish. It's a little
Or, can we make sure the rename is durable by calling fsync before
returning the fd, as a patch attached here?
Regards,
Yugo Nagata
> pessimistic to do that unconditionally, though, as then you have to
> wait even for rename operations for segment files later than the one
> you're opening, so I thought about how to skip waiting in that case --
> see 0002. I'm not sure if it's worth worrying about or not.
--
Yugo NAGATA
diff --git a/src/backend/access/transam/xlog.c b/src/backend/access/transam/xlog.c
index 478377c4a2..862109ca3b 100644
--- a/src/backend/access/transam/xlog.c
+++ b/src/backend/access/transam/xlog.c
@@ -3062,7 +3062,11 @@ XLogFileInitInternal(XLogSegNo logsegno, TimeLineID logtli,
errmsg("could not open file \"%s\": %m", path)));
}
else
+ {
+ fsync_fname_ext(path, false, false, ERROR);
+ fsync_parent_path(path, ERROR);
return fd;
+ }
/*
* Initialize an empty (all zeroes) segment. NOTE: it is possible that
Re: Small fix on COPY ON_ERROR document
On Sat, 3 Feb 2024 01:51:56 +0200 Alexander Korotkov wrote: > On Fri, Feb 2, 2024 at 10:07 PM David G. Johnston > wrote: > > On Thu, Feb 1, 2024 at 11:59 PM Yugo NAGATA wrote: > >> > >> > >> I attached a updated patch including fixes you pointed out above. > >> > > > > Removed "which"; changed "occupying" to "occupy" > > Removed on of the two "amounts" > > Changed "unacceptable to the input function" to just "converting" as that > > is what the average reader is more likely to be thinking. > > The rest was good. > > Thanks to everybody in the thread. > Pushed. Thanks! > > -- > Regards, > Alexander Korotkov -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 1 Feb 2024 17:59:56 +0800 jian he wrote: > On Thu, Feb 1, 2024 at 12:45 PM Yugo NAGATA wrote: > > > > Here is a updated patch, v6. > > v6 patch looks good. Thank you for your review and updating the status to RwC! Regards, Yugo Nagata -- Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Mon, 05 Feb 2024 11:28:59 +0900
torikoshia wrote:
> > Based on this, I've made a patch.
> > based on COPY Synopsis: ON_ERROR 'error_action'
> > on_error 'null', the keyword NULL should be single quoted.
>
> As you mentioned, single quotation seems a little odd..
>
> I'm not sure what is the best name and syntax for this feature, but
> since current error_action are verbs('stop' and 'ignore'), I feel 'null'
> might not be appropriate.
I am not in favour of using 'null' either, so I suggested to use
"set_to_null" or more generic syntax like "set_to (col, val)" in my
previous post[1], although I'm not convinced what is the best either.
[1]
https://www.postgresql.org/message-id/20240129172858.ccb6c77c3be95a295e2b2b44%40sraoss.co.jp
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Tue, 06 Feb 2024 09:39:09 +0900 (JST)
Kyotaro Horiguchi wrote:
> At Mon, 5 Feb 2024 17:22:56 +0900, Yugo NAGATA wrote in
> > On Mon, 05 Feb 2024 11:28:59 +0900
> > torikoshia wrote:
> >
> > > > Based on this, I've made a patch.
> > > > based on COPY Synopsis: ON_ERROR 'error_action'
> > > > on_error 'null', the keyword NULL should be single quoted.
> > >
> > > As you mentioned, single quotation seems a little odd..
> > >
> > > I'm not sure what is the best name and syntax for this feature, but
> > > since current error_action are verbs('stop' and 'ignore'), I feel 'null'
> > > might not be appropriate.
> >
> > I am not in favour of using 'null' either, so I suggested to use
> > "set_to_null" or more generic syntax like "set_to (col, val)" in my
> > previous post[1], although I'm not convinced what is the best either.
> >
> > [1]
> > https://www.postgresql.org/message-id/20240129172858.ccb6c77c3be95a295e2b2b44%40sraoss.co.jp
>
> Tom sugggested using a separate option, and I agree with the
> suggestion. Taking this into consideration, I imagined something like
> the following, for example. Although I'm not sure we are actually
> going to do whole-tuple replacement, the action name in this example
> has the suffix '-column'.
>
> COPY (on_error 'replace-colomn', replacement 'null') ..
Thank you for your information. I've found a post[1] you mentioned,
where adding a separate option for error log destination was suggested.
Considering consistency with other options, adding a separate option
would be better if we want to specify a value to replace the invalid
value, without introducing a complex syntax that allows options with
more than one parameters. Maybe, if we allow to use values for the
replacement other than NULL, we have to also add a option to specify
a column (or a type) for each replacement value. Or, we may add a
option to specify a list of replacement values as many as the number of
columns, each of whose default is NULL.
Anyway, I prefer 'replace" (or 'set_to') to just 'null' as the option
value.
[1] https://www.postgresql.org/message-id/2070915.1705527477%40sss.pgh.pa.us
Regards,
Yugo Nagata
> regards.
>
> --
> Kyotaro Horiguchi
> NTT Open Source Software Center
--
Yugo NAGATA
Re: Change COPY ... ON_ERROR ignore to ON_ERROR ignore_row
On Mon, 5 Feb 2024 14:26:46 +0800
jian he wrote:
> On Mon, Feb 5, 2024 at 10:29 AM torikoshia wrote:
> >
> > Hi,
> >
> > On 2024-02-03 15:22, jian he wrote:
> > > The idea of on_error is to tolerate errors, I think.
> > > if a column has a not null constraint, let it cannot be used with
> > > (on_error 'null')
> >
> > > + /*
> > > +* we can specify on_error 'null', but it can only apply to
> > > columns
> > > +* don't have not null constraint.
> > > + */
> > > + if (att->attnotnull && cstate->opts.on_error ==
> > > COPY_ON_ERROR_NULL)
> > > + ereport(ERROR,
> > > + (errcode(ERRCODE_BAD_COPY_FILE_FORMAT),
> > > +errmsg("copy on_error 'null' cannot be used with
> > > not null constraint column")));
> >
> > This means we cannot use ON_ERROR 'null' even when there is one column
> > which have NOT NULL constraint, i.e. primary key, right?
> > IMHO this is strong constraint and will decrease the opportunity to use
> > this feature.
>
> I don't want to fail in the middle of bulk inserts,
> so I thought immediately erroring out would be a great idea.
> Let's see what other people think.
I also think this restriction is too strong because it is very
common that a table has a primary key, unless there is some way
to specify columns that can be set to NULL. Even when ON_ERROR
is specified, any constraint violation errors cannot be generally
ignored, so we cannot elimiate the posibility COPY FROM fails in
the middle due to invalid data, anyway.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: pgbnech: allow to cancel queries during benchmark
On Wed, 24 Jan 2024 22:17:44 +0900 Yugo NAGATA wrote: > Attached is the updated patch, v6. Currently, on non-Windows, SIGINT is received only by thread #0. CancelRequested is checked during the loop in the thread, and queries are cancelled if it is set. However, once thread #0 exits the loop due to some runtime error and starts waiting in pthread_join, there is no opportunity to cancel queries run by other threads. In addition, if -C option is specified, connections are created for each transaction, so cancel objects (PGcancel) also have to be recreated at each time in each thread. However, these cancel objects are used in a specific thread to perform cancel for all queries, which is not safe because a thread refers to objects updated by other threads. I think the first problem would be addressed by any of followings. (1a) Perform cancels in the signal handler. The signal handler will be called even while the thread 0 is blocked in pthread_join. This is safe because PQcancel is callable from a signal handler. (1b) Create an additional dedicated thread that calls sigwait on SIGINT and performs query cancel. As far as I studied, creating such dedicated thread calling sigwait is a typical way to handle signal in multi-threaded programming. (1c) Each thread performs cancel for queries run by each own, rather than that thread 0 cancels all queries. For the purpose, pthread_kill might be used to interrupt threads blocked in wait_on_socket_set. The second one would be addressed by any of followings. (2a) Use critical section when accessing PGcancel( e.g by using pthread_mutex (non-Windows) or EnterCriticalSection (Windows)). On non-Windows, we cannot use this way when calling PQcancel in a signal handler ((1a) above) because acquiring a lock is not re-entrant. (2b) Perform query cancel in each thread that has created the connection (same as (1c) above). Considering both, possible combination would be either (1b)&(2a) or (1c)&(2b). I would prefer the former way, because creating the dedicated thread handling SIGINT signal and canceling all queries seems simpler and safer than calling pthread_kill in the SIGINT signal handler to send another signal to other threads. I'll update the patch in this way soon. Regards, Yugo Nagata > > > Best reagards, > > -- > > Tatsuo Ishii > > SRA OSS LLC > > English: http://www.sraoss.co.jp/index_en/ > > Japanese:http://www.sraoss.co.jp > > > > > > > -- > Yugo NAGATA -- Yugo NAGATA
Re: Rename setup_cancel_handler in pg_dump
On Wed, 7 Feb 2024 22:59:48 +0100 Daniel Gustafsson wrote: > > On 1 Feb 2024, at 02:21, Yugo NAGATA wrote: > > On Tue, 30 Jan 2024 13:44:28 +0100 > > Daniel Gustafsson wrote: > > >> -setup_cancel_handler(void) > >> +pg_dump_setup_cancel_handler(void) > >> > >> We don't have any other functions prefixed with pg_dump_, based on the > >> naming > >> of the surrounding code in the file I wonder if set_cancel_handler is a > >> more > >> appropriate name? > > > > Agreed. Here is a updated patch. > > Sleeping on it I still think this is a good idea, and hearing no objections I > went ahead with this. Thanks for the patch! Thank you! Regards, Yugo Nagata > > -- > Daniel Gustafsson > -- Yugo NAGATA
Small fix on query_id_enabled
Hi,
I found the comment on query_id_enabled looks inaccurate because this is
never set to true when compute_query_id is ON.
/* True when compute_query_id is ON, or AUTO and a module requests them */
bool query_id_enabled = false;
Should we fix this as following (just fixing the place of a comma) ?
/* True when compute_query_id is ON or AUTO, and a module requests them */
Also, I think the name is a bit confusing for the same reason, that is,
query_id_enabled may be false even when query id is computed in fact.
Actually, this does not matter because we use IsQueryIdEnabled to check
if query id is enabled, instead of referring to a global variable
(query_id_enabled or compute_query_id). But, just for making a code a bit
more readable, how about renaming this to query_id_required which seems to
stand for the meaning more correctly?
I attached a patch for above fixes.
Although renaming might not be acceptable since changing global variables
may affect third party tools, I think the comment should be fixed at least.
IMHO, it seems better to make this variable static not to be accessed directly.
However, I left it as is because this is used in a static inline function.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c
index e489bfceb5..e4fbcf0d9f 100644
--- a/src/backend/nodes/queryjumblefuncs.c
+++ b/src/backend/nodes/queryjumblefuncs.c
@@ -42,8 +42,8 @@
/* GUC parameters */
int compute_query_id = COMPUTE_QUERY_ID_AUTO;
-/* True when compute_query_id is ON, or AUTO and a module requests them */
-bool query_id_enabled = false;
+/* True when compute_query_id is ON or AUTO, and a module requests them */
+bool query_id_required = false;
static void AppendJumble(JumbleState *jstate,
const unsigned char *item, Size size);
@@ -145,7 +145,7 @@ void
EnableQueryId(void)
{
if (compute_query_id != COMPUTE_QUERY_ID_OFF)
- query_id_enabled = true;
+ query_id_required = true;
}
/*
diff --git a/src/backend/postmaster/postmaster.c b/src/backend/postmaster/postmaster.c
index a066800a1c..4bcaf6d71c 100644
--- a/src/backend/postmaster/postmaster.c
+++ b/src/backend/postmaster/postmaster.c
@@ -532,7 +532,7 @@ typedef struct
pg_time_t first_syslogger_file_time;
bool redirection_done;
bool IsBinaryUpgrade;
- bool query_id_enabled;
+ bool query_id_required;
int max_safe_fds;
int MaxBackends;
#ifdef WIN32
@@ -6103,7 +6103,7 @@ save_backend_variables(BackendParameters *param, Port *port, BackgroundWorker *w
param->redirection_done = redirection_done;
param->IsBinaryUpgrade = IsBinaryUpgrade;
- param->query_id_enabled = query_id_enabled;
+ param->query_id_required = query_id_required;
param->max_safe_fds = max_safe_fds;
param->MaxBackends = MaxBackends;
@@ -6348,7 +6348,7 @@ restore_backend_variables(BackendParameters *param, Port **port, BackgroundWorke
redirection_done = param->redirection_done;
IsBinaryUpgrade = param->IsBinaryUpgrade;
- query_id_enabled = param->query_id_enabled;
+ query_id_required = param->query_id_required;
max_safe_fds = param->max_safe_fds;
MaxBackends = param->MaxBackends;
diff --git a/src/include/nodes/queryjumble.h b/src/include/nodes/queryjumble.h
index f1c55c8067..3b2e1f8018 100644
--- a/src/include/nodes/queryjumble.h
+++ b/src/include/nodes/queryjumble.h
@@ -67,7 +67,7 @@ extern const char *CleanQuerytext(const char *query, int *location, int *len);
extern JumbleState *JumbleQuery(Query *query);
extern void EnableQueryId(void);
-extern PGDLLIMPORT bool query_id_enabled;
+extern PGDLLIMPORT bool query_id_required;
/*
* Returns whether query identifier computation has been enabled, either
@@ -80,7 +80,7 @@ IsQueryIdEnabled(void)
return false;
if (compute_query_id == COMPUTE_QUERY_ID_ON)
return true;
- return query_id_enabled;
+ return query_id_required;
}
#endif /* QUERYJUMBLE_H */
Re: Small fix on query_id_enabled
On Sat, 10 Feb 2024 10:19:15 +0900 Michael Paquier wrote: > On Fri, Feb 09, 2024 at 04:37:23PM +0800, Julien Rouhaud wrote: > > On Fri, Feb 09, 2024 at 03:38:23PM +0900, Yugo NAGATA wrote: > >> Also, I think the name is a bit confusing for the same reason, that is, > >> query_id_enabled may be false even when query id is computed in fact. > >> > >> Actually, this does not matter because we use IsQueryIdEnabled to check > >> if query id is enabled, instead of referring to a global variable > >> (query_id_enabled or compute_query_id). But, just for making a code a bit > >> more readable, how about renaming this to query_id_required which seems to > >> stand for the meaning more correctly? > > > > -1 for renaming to avoid breaking extensions that might access it. We > > should > > simply document for compute_query_id and query_id_enabled declaration that > > one > > should instead use IsQueryIdEnabled() if they're interested in whether the > > core > > queryid are computed or not. > > Agreed. A renaming would involve more pain than gain. Improving the > comments around how to all that would be good enough, my 2c. Thank you both for your comments. I agreed with not renaming it. I attached a updated patch that adds comments noting to use IsQueryIdEnabled() instead of accessing the variables directly. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/backend/nodes/queryjumblefuncs.c b/src/backend/nodes/queryjumblefuncs.c index e489bfceb5..82f725baaa 100644 --- a/src/backend/nodes/queryjumblefuncs.c +++ b/src/backend/nodes/queryjumblefuncs.c @@ -42,7 +42,13 @@ /* GUC parameters */ int compute_query_id = COMPUTE_QUERY_ID_AUTO; -/* True when compute_query_id is ON, or AUTO and a module requests them */ +/* + * True when compute_query_id is ON or AUTO, and a module requests them. + * + * Note that IsQueryIdEnabled() should be used instead of checking + * query_id_enabled or compute_query_id directly when we want to know + * whether query identifiers are computed in the core or not. + */ bool query_id_enabled = false; static void AppendJumble(JumbleState *jstate,
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Hello Fabien,
On Sat, 12 Mar 2022 15:54:54 +0100 (CET)
Fabien COELHO wrote:
> Hello Yugo-san,
>
> About Pgbench error handling v16:
Thank you for your review! I attached the updated patches.
> This patch set needs a minor rebase because of 506035b0. Otherwise, patch
> compiles, global and local "make check" are ok. Doc generation is ok.
I rebased it.
> ## About v16-2
> English: "he will be aborted" -> "it will be aborted".
Fixed.
> I'm still not sure I like the "failure detailed" option, ISTM that the report
> could be always detailed. That would remove some complexity and I do not think
> that people executing a bench with error handling would mind having the
> details.
> No big deal.
I didn't change it because I think those who don't expect any failures using a
well designed script may not need details of failures. I think reporting such
details will be required only for benchmarks where any failures are expected.
> printVerboseErrorMessages: I'd make the buffer static and initialized only
> once
> so that there is no significant malloc/free cycle involved when calling the
> function.
OK. I fixed printVerboseErrorMessages to use a static variable.
> advanceConnectionState: I'd really prefer not to add new variables (res,
> status)
> in the loop scope, and only declare them when actually needed in the state
> branches,
> so as to avoid any unwanted interaction between states.
I fixed to declare the variables in the case statement blocks.
> typo: "fullowing" -> "following"
fixed.
> Pipeline cleaning: the advance function is already s long, I'd put that
> in a
> separate function and call it.
Ok. I made a new function "discardUntilSync" for the pipeline cleaning.
> I think that the report should not remove data when they are 0, otherwise it
> makes
> it harder to script around it (in failures_detailed on line 6284).
I fixed to report both serialization and deadlock failures always even when
they are 0.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From b4360d3c03013c86e1e62247f1c3c1378aacc38d Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 26 May 2021 16:58:36 +0900
Subject: [PATCH v17 1/2] Pgbench errors: use the Variables structure for
client variables
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++---
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 000ffc4a5c..ab2c5dfc5f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -289,6 +289,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -306,6 +312,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1398,39 +1420,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
Hi Ishii-san, On Sun, 20 Mar 2022 09:52:06 +0900 (JST) Tatsuo Ishii wrote: > Hi Yugo, > > I have looked into the patch and I noticed that linkend=... endterm=...> is used in pgbench.sgml. e.g. > > > > AFAIK this is the only place where "endterm" is used. In other places > "link" tag is used instead: Thank you for pointing out it. I've checked other places using referring to , and found that "xreflabel"s are used in such tags. So, I'll fix it in this style. Regards, Yugo Nagata -- Yugo NAGATA
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Sun, 20 Mar 2022 16:11:43 +0900 (JST) Tatsuo Ishii wrote: > > Hi Yugo, > > > > I tested with serialization error scenario by setting: > > default_transaction_isolation = 'repeatable read' > > The result was: > > > > $ pgbench -t 10 -c 10 --max-tries=10 test > > transaction type: > > scaling factor: 10 > > query mode: simple > > number of clients: 10 > > number of threads: 1 > > maximum number of tries: 10 > > number of transactions per client: 10 > > number of transactions actually processed: 100/100 > > number of failed transactions: 0 (0.000%) > > number of transactions retried: 35 (35.000%) > > total number of retries: 74 > > latency average = 5.306 ms > > initial connection time = 15.575 ms > > tps = 1884.516810 (without initial connection time) > > > > I had hard time to understand what those numbers mean: > > number of transactions retried: 35 (35.000%) > > total number of retries: 74 > > > > It seems "total number of retries" matches with the number of ERRORs > > reported in PostgreSQL. Good. What I am not sure is "number of > > transactions retried". What does this mean? > > Oh, ok. I see it now. It turned out that "number of transactions > retried" does not actually means the number of transactions > rtried. Suppose pgbench exectutes following in a session: > > BEGIN;-- transaction A starts > : > (ERROR) > ROLLBACK; -- transaction A aborts > > (retry) > > BEGIN;-- transaction B starts > : > (ERROR) > ROLLBACK; -- transaction B aborts > > (retry) > > BEGIN;-- transaction C starts > : > END; -- finally succeeds > > In this case "total number of retries:" = 2 and "number of > transactions retried:" = 1. In this patch transactions A, B and C are > regarded as "same" transaction, so the retried transaction count > becomes 1. But it's confusing to use the language "transaction" here > because A, B and C are different transactions. I would think it's > better to use different language instead of "transaction", something > like "cycle"? i.e. > > number of cycles retried: 35 (35.000%) In the original patch by Marina Polyakova it was "number of retried", but I changed it to "number of transactions retried" is because I felt it was confusing with "number of retries". I chose the word "transaction" because a transaction ends in any one of successful commit , skipped, or failure, after possible retries. Well, I agree with that it is somewhat confusing wording. If we can find nice word to resolve the confusion, I don't mind if we change the word. Maybe, we can use "executions" as well as "cycles". However, I am not sure that the situation is improved by using such word because what such word exactly means seems to be still unclear for users. Another idea is instead reporting only "the number of successfully retried transactions" that does not include "failed transactions", that is, transactions failed after retries, like this; number of transactions actually processed: 100/100 number of failed transactions: 0 (0.000%) number of successfully retried transactions: 35 (35.000%) total number of retries: 74 The meaning is clear and there seems to be no confusion. Regards, Yugo Nagata -- Yugo NAGATA
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Tue, 22 Mar 2022 09:08:15 +0900
Yugo NAGATA wrote:
> Hi Ishii-san,
>
> On Sun, 20 Mar 2022 09:52:06 +0900 (JST)
> Tatsuo Ishii wrote:
>
> > Hi Yugo,
> >
> > I have looked into the patch and I noticed that > linkend=... endterm=...> is used in pgbench.sgml. e.g.
> >
> >
> >
> > AFAIK this is the only place where "endterm" is used. In other places
> > "link" tag is used instead:
>
> Thank you for pointing out it.
>
> I've checked other places using referring to , and found
> that "xreflabel"s are used in such tags. So, I'll fix it
> in this style.
I attached the updated patch. I also fixed the following paragraph which I had
forgotten to fix in the previous patch.
The first seven lines report some of the most important parameter settings.
The sixth line reports the maximum number of tries for transactions with
serialization or deadlock errors
Regards,
Yugo Nagata
--
Yugo NAGATA
>From b9f993e81836c4379478809da0e690023c319038 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 26 May 2021 16:58:36 +0900
Subject: [PATCH v18 1/2] Pgbench errors: use the Variables structure for
client variables
This is most important when it is used to reset client variables during the
repeating of transactions after serialization/deadlock failures.
Don't allocate Variable structs one by one. Instead, add a constant margin each
time it overflows.
---
src/bin/pgbench/pgbench.c | 163 +++---
1 file changed, 100 insertions(+), 63 deletions(-)
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index 000ffc4a5c..ab2c5dfc5f 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -289,6 +289,12 @@ const char *progname;
volatile bool timer_exceeded = false; /* flag from signal handler */
+/*
+ * We don't want to allocate variables one by one; for efficiency, add a
+ * constant margin each time it overflows.
+ */
+#define VARIABLES_ALLOC_MARGIN 8
+
/*
* Variable definitions.
*
@@ -306,6 +312,24 @@ typedef struct
PgBenchValue value; /* actual variable's value */
} Variable;
+/*
+ * Data structure for client variables.
+ */
+typedef struct
+{
+ Variable *vars; /* array of variable definitions */
+ int nvars; /* number of variables */
+
+ /*
+ * The maximum number of variables that we can currently store in 'vars'
+ * without having to reallocate more space. We must always have max_vars >=
+ * nvars.
+ */
+ int max_vars;
+
+ bool vars_sorted; /* are variables sorted by name? */
+} Variables;
+
#define MAX_SCRIPTS 128 /* max number of SQL scripts allowed */
#define SHELL_COMMAND_SIZE 256 /* maximum size allowed for shell command */
@@ -460,9 +484,7 @@ typedef struct
int command; /* command number in script */
/* client variables */
- Variable *variables; /* array of variable definitions */
- int nvariables; /* number of variables */
- bool vars_sorted; /* are variables sorted by name? */
+ Variables variables;
/* various times about current transaction in microseconds */
pg_time_usec_t txn_scheduled; /* scheduled start time of transaction */
@@ -1398,39 +1420,39 @@ compareVariableNames(const void *v1, const void *v2)
/* Locate a variable by name; returns NULL if unknown */
static Variable *
-lookupVariable(CState *st, char *name)
+lookupVariable(Variables *variables, char *name)
{
Variable key;
/* On some versions of Solaris, bsearch of zero items dumps core */
- if (st->nvariables <= 0)
+ if (variables->nvars <= 0)
return NULL;
/* Sort if we have to */
- if (!st->vars_sorted)
+ if (!variables->vars_sorted)
{
- qsort((void *) st->variables, st->nvariables, sizeof(Variable),
+ qsort((void *) variables->vars, variables->nvars, sizeof(Variable),
compareVariableNames);
- st->vars_sorted = true;
+ variables->vars_sorted = true;
}
/* Now we can search */
key.name = name;
return (Variable *) bsearch((void *) &key,
-(void *) st->variables,
-st->nvariables,
+(void *) variables->vars,
+variables->nvars,
sizeof(Variable),
compareVariableNames);
}
/* Get the value of a variable, in string form; returns NULL if unknown */
static char *
-getVariable(CState *st, char *name)
+getVariable(Variables *variables, char *name)
{
Variable *var;
char stringform[64];
- var = lookupVariable(st, name);
+ var = lookupVariable(variables, name);
if (var == NULL)
return NULL; /* not found */
@@ -1562,21 +1584,37 @@ valid_variable_name(const char *name)
return true;
}
+/*
+ * Make sure there is enough space for 'needed' more variable in the variables
+ * array.
+ */
+static void
+enlargeVariables(Variables *variables, int needed)
+{
+ /* total number of variables required no
Re: Tab completion for ALTER MATERIALIZED VIEW ... SET ACCESS METHOD
On Sat, 19 Mar 2022 19:31:59 +0900 Michael Paquier wrote: > On Fri, Mar 18, 2022 at 03:13:05PM +0900, Michael Paquier wrote: > > Thanks. This looks rather sane to me. I'll split things into 3 > > commits in total, as of the psql completion, SET TABLESPACE and SET > > ACCESS METHOD. The first and third patches are only for HEAD, while > > the documentation hole with SET TABLESPACE should go down to v10. > > Backpatching the tests of ALTER MATERIALIZED VIEW ALL IN TABLESPACE > > would not hurt, either, as there is zero coverage for that now. > > I have applied the set, after splitting things mostly as mentioned > upthread: > - The doc change for SET TABLESPACE on ALTER MATVIEW has been > backpatched. > - The regression tests for SET TABLESPACE have been applied on HEAD, > as these are independent of the rest, good on their own. > - All the remaining parts for SET ACCESS METHOD (psql tab completion, > tests and docs) have been merged together on HEAD. I could not > understand why the completion done after SET ACCESS METHOD was not > grouped with the other parts for ALTER MATVIEW, so I have moved the > new entry there, and I have added a test checking after an error for > multiple subcommands, while on it. > > Thanks, Nagata-san! Thank you! -- Yugo NAGATA
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Wed, 23 Mar 2022 14:26:54 -0400
Tom Lane wrote:
> Tatsuo Ishii writes:
> > The patch Pushed. Thank you!
>
> My hoary animal prairiedog doesn't like this [1]:
>
> # Failed test 'concurrent update with retrying stderr /(?s-xim:client (0|1)
> got an error in command 3 \\(SQL\\) of script 0; ERROR: could not serialize
> access due to concurrent update\\b.*\\g1)/'
> # at t/001_pgbench_with_server.pl line 1229.
> # 'pgbench: pghost: /tmp/nhghgwAoki pgport: 58259 nclients:
> 2 nxacts: 1 dbName: postgres
> ...
> # pgbench: client 0 got an error in command 3 (SQL) of script 0; ERROR:
> could not serialize access due to concurrent update
> ...
> # '
> # doesn't match '(?s-xim:client (0|1) got an error in command 3 \\(SQL\\)
> of script 0; ERROR: could not serialize access due to concurrent
> update\\b.*\\g1)'
> # Looks like you failed 1 test of 425.
>
> I'm not sure what the "\\b.*\\g1" part of this regex is meant to
> accomplish, but it seems to be assuming more than it should
> about the output format of TAP messages.
I have edited the test code from the original patch by mistake, but
I could not realize because the test works in my machine without any
errors somehow.
I attached a patch to fix the test as was in the original patch, where
backreferences are used to check retry of the same query.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pgbench/t/001_pgbench_with_server.pl b/src/bin/pgbench/t/001_pgbench_with_server.pl
index d173ceae7a..3eb5905e5a 100644
--- a/src/bin/pgbench/t/001_pgbench_with_server.pl
+++ b/src/bin/pgbench/t/001_pgbench_with_server.pl
@@ -1222,7 +1222,8 @@ local $ENV{PGOPTIONS} = "-c default_transaction_isolation=repeatable\\ read";
# Check that we have a serialization error and the same random value of the
# delta variable in the next try
my $err_pattern =
-"client (0|1) got an error in command 3 \\(SQL\\) of script 0; "
+ "(client (0|1) sending UPDATE xy SET y = y \\+ -?\\d+\\b).*"
+ . "client \\g2 got an error in command 3 \\(SQL\\) of script 0; "
. "ERROR: could not serialize access due to concurrent update\\b.*"
. "\\g1";
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Fri, 25 Mar 2022 09:14:00 +0900 (JST) Tatsuo Ishii wrote: > > Note that the \\g2 just above also needs to be changed. > > Oops. Thanks. New patch attached. Test has passed on my machine. This patch works for me. I think it is ok to use \N instead of \gN. Regards, Yugo Nagata -- Yugo NAGATA
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Sun, 27 Mar 2022 15:28:41 +0900 (JST)
Tatsuo Ishii wrote:
> > This patch has caused the PDF documentation to fail to build cleanly:
> >
> > [WARN] FOUserAgent - The contents of fo:block line 1 exceed the available
> > area in the inline-progression direction by more than 50 points. (See
> > position 125066:375)
> >
> > It's complaining about this:
> >
> >
> > interval_start
> > num_transactions
> > sum_latency
> > sum_latency_2
> > min_latency
> > max_latency {
> > failures |
> > serialization_failures
> > deadlock_failures }
> > sum_lag sum_lag_2
> > min_lag max_lag
> > skipped
> > retried
> > retries
> >
> >
> > which runs much too wide in HTML format too, even though that toolchain
> > doesn't tell you so.
>
> Yeah.
>
> > We could silence the warning by inserting an arbitrary line break or two,
> > or refactoring the syntax description into multiple parts. Either way
> > seems to create a risk of confusion.
>
> I think we can fold the line nicely. Here is the rendered image.
>
> Before:
> interval_start num_transactions sum_latency sum_latency_2 min_latency
> max_latency { failures | serialization_failures deadlock_failures } [ sum_lag
> sum_lag_2 min_lag max_lag [ skipped ] ] [ retried retries ]
>
> After:
> interval_start num_transactions sum_latency sum_latency_2 min_latency
> max_latency
> { failures | serialization_failures deadlock_failures } [ sum_lag sum_lag_2
> min_lag max_lag [ skipped ] ] [ retried retries ]
>
> Note that before it was like this:
>
> interval_start num_transactions sum_latency sum_latency_2 min_latency
> max_latency [ sum_lag sum_lag_2 min_lag max_lag [ skipped ] ]
>
> So newly added items are "{ failures | serialization_failures
> deadlock_failures }" and " [ retried retries ]".
>
> > TBH, I think the *real* problem is that the complexity of this log format
> > has blown past "out of hand". Can't we simplify it? Who is really going
> > to use all these numbers? I pity the poor sucker who tries to write a
> > log analysis tool that will handle all the variants.
>
> Well, the extra logging items above only appear when the retry feature
> is enabled. For those who do not use the feature the only new logging
> item is "failures". For those who use the feature, the extra logging
> items are apparently necessary. For example if we write an application
> using repeatable read or serializable transaction isolation mode,
> retrying failed transactions due to srialization error is an essential
> technique. Also the retry rate of transactions will deeply affect the
> performance and in such use cases the newly added items will be
> precisou information. I would suggest leave the log items as it is.
>
> Patch attached.
Even applying this patch, "make postgres-A4.pdf" arises the warning on my
machine. After some investigations, I found that previous document had a break
after 'num_transactions', but it has been removed due to this commit. So,
I would like to get back this as it was. I attached the patch.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index ebdb4b3f46..4437d5ef53 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -2401,7 +2401,7 @@ END;
format is used for the log files:
-interval_start num_transactions sum_latency sum_latency_2 min_latency max_latency { failures | serialization_failures deadlock_failures } sum_lag sum_lag_2 min_lag max_lag skippedretried retries
+interval_start num_transactions&zwsp sum_latency sum_latency_2 min_latency max_latency { failures | serialization_failures deadlock_failures } sum_lag sum_lag_2 min_lag max_lag skippedretried retries
where
Re: pgbench: using prepared BEGIN statement in a pipeline could cause an error
On Fri, 25 Mar 2022 16:19:54 -0400
Tom Lane wrote:
> Fabien COELHO writes:
> >> [...] One way to avoid these errors is to send Parse messages before
> >> pipeline mode starts. I attached a patch to fix to prepare commands at
> >> starting of a script instead of at the first execution of the command.
>
> > ISTM that moving prepare out of command execution is a good idea, so I'm
> > in favor of this approach: the code is simpler and cleaner.
> > ISTM that a minor impact is that the preparation is not counted in the
> > command performance statistics. I do not think that it is a problem, even
> > if it would change detailed results under -C -r -M prepared.
>
> I am not convinced this is a great idea. The current behavior is that
> a statement is not prepared until it's about to be executed, and I think
> we chose that deliberately to avoid semantic differences between prepared
> and not-prepared mode. For example, if a script looks like
>
> CREATE FUNCTION foo(...) ...;
> SELECT foo(...);
> DROP FUNCTION foo;
>
> trying to prepare the SELECT in advance would lead to failure.
>
> We could perhaps get away with preparing the commands within a pipeline
> just before we start to execute the pipeline, but it looks to me like
> this patch tries to prepare the entire script in advance.
>
Well, the semantic differences is already in the current behavior.
Currently, pgbench fails to execute the above script in prepared mode
because it prepares the entire script in advance just before the first
command execution. This patch does not change the semantic.
> BTW, the cfbot says the patch is failing to apply anyway ...
> I think it was sideswiped by 4a39f87ac.
I attached the rebased patch.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/pgbench/pgbench.c b/src/bin/pgbench/pgbench.c
index acf3e56413..bf8fecf219 100644
--- a/src/bin/pgbench/pgbench.c
+++ b/src/bin/pgbench/pgbench.c
@@ -3074,6 +3074,30 @@ chooseScript(TState *thread)
return i - 1;
}
+/* Prepare SQL commands in the chosen script */
+static void
+prepareCommands(CState *st)
+{
+ int j;
+ Command **commands = sql_script[st->use_file].commands;
+
+ for (j = 0; commands[j] != NULL; j++)
+ {
+ PGresult *res;
+ char name[MAX_PREPARE_NAME];
+
+ if (commands[j]->type != SQL_COMMAND)
+ continue;
+ preparedStatementName(name, st->use_file, j);
+ res = PQprepare(st->con, name,
+ commands[j]->argv[0], commands[j]->argc - 1, NULL);
+ if (PQresultStatus(res) != PGRES_COMMAND_OK)
+ pg_log_error("%s", PQerrorMessage(st->con));
+ PQclear(res);
+ }
+ st->prepared[st->use_file] = true;
+}
+
/* Send a SQL command, using the chosen querymode */
static bool
sendCommand(CState *st, Command *command)
@@ -3107,42 +3131,6 @@ sendCommand(CState *st, Command *command)
char name[MAX_PREPARE_NAME];
const char *params[MAX_ARGS];
- if (!st->prepared[st->use_file])
- {
- int j;
- Command **commands = sql_script[st->use_file].commands;
-
- for (j = 0; commands[j] != NULL; j++)
- {
-PGresult *res;
-char name[MAX_PREPARE_NAME];
-
-if (commands[j]->type != SQL_COMMAND)
- continue;
-preparedStatementName(name, st->use_file, j);
-if (PQpipelineStatus(st->con) == PQ_PIPELINE_OFF)
-{
- res = PQprepare(st->con, name,
- commands[j]->argv[0], commands[j]->argc - 1, NULL);
- if (PQresultStatus(res) != PGRES_COMMAND_OK)
- pg_log_error("%s", PQerrorMessage(st->con));
- PQclear(res);
-}
-else
-{
- /*
- * In pipeline mode, we use asynchronous functions. If a
- * server-side error occurs, it will be processed later
- * among the other results.
- */
- if (!PQsendPrepare(st->con, name,
- commands[j]->argv[0], commands[j]->argc - 1, NULL))
- pg_log_error("%s", PQerrorMessage(st->con));
-}
- }
- st->prepared[st->use_file] = true;
- }
-
getQueryParams(&st->variables, command, params);
preparedStatementName(name, st->use_file, st->command);
@@ -3619,6 +3607,20 @@ advanceConnectionState(TState *thread, CState *st, StatsData *agg)
memset(st->prepared, 0, sizeof(st->prepared));
}
+/*
+ * Prepare SQL commands if needed.
+ *
+ * We should send Parse messages before executing meta commands
+ * especially /startpipeline. If a Parse message is sent in
+ * pipeline mode, a transaction starts before BEGIN is sent, and
+ * it could be a problem. For example, "BEGIN ISOLATION LEVEL
+ * SERIALIZABLE" is sent after a transaction starts, the error
+ * "ERROR: SET TRANSACTION ISOLATION LEVEL must be called before any query"
+ * occurs.
+ */
+if (querymode == QUERY_PREPARED &&
Re: [HACKERS] WIP aPatch: Pgbench Serialization and deadlock errors
On Mon, 28 Mar 2022 12:17:13 +0900 (JST)
Tatsuo Ishii wrote:
> > Even applying this patch, "make postgres-A4.pdf" arises the warning on my
> > machine. After some investigations, I found that previous document had a
> > break
> > after 'num_transactions', but it has been removed due to this commit.
>
> Yes, your patch removed "&zwsp;".
>
> > So,
> > I would like to get back this as it was. I attached the patch.
>
> This produces errors. Needs ";" postfix?
Oops. Yes, it needs ';'. Also, I found another "&zwsp;" dropped.
I attached the fixed patch.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml
index ebdb4b3f46..b16a5b9b7b 100644
--- a/doc/src/sgml/ref/pgbench.sgml
+++ b/doc/src/sgml/ref/pgbench.sgml
@@ -2401,7 +2401,7 @@ END;
format is used for the log files:
-interval_start num_transactions sum_latency sum_latency_2 min_latency max_latency { failures | serialization_failures deadlock_failures } sum_lag sum_lag_2 min_lag max_lag skippedretried retries
+interval_start num_transactions&zwsp; sum_latency sum_latency_2 min_latency max_latency&zwsp; { failures | serialization_failures deadlock_failures } sum_lag sum_lag_2 min_lag max_lag skippedretried retries
where
Re: Implementing Incremental View Maintenance
On Wed, 25 Nov 2020 18:00:16 +0300 Konstantin Knizhnik wrote: > > > On 25.11.2020 16:06, Yugo NAGATA wrote: > > On Wed, 25 Nov 2020 15:16:05 +0300 > > Konstantin Knizhnik wrote: > > > >> > >> On 24.11.2020 13:11, Yugo NAGATA wrote: > >>>> I wonder if it is possible to somehow use predicate locking mechanism of > >>>> Postgres to avoid this anomalies without global lock? > >>> You mean that, ,instead of using any table lock, if any possibility of the > >>> anomaly is detected using predlock mechanism then abort the transaction? > >> Yes. If both transactions are using serializable isolation level, then > >> lock is not needed, isn't it? > >> So at least you can add yet another simple optimization: if transaction > >> has serializable isolation level, > >> then exclusive lock is not required. > > As long as we use the trigger approach, we can't handle concurrent view > > maintenance > > in either repeatable read or serializable isolation level. It is because > > one > > transaction (R= R+dR) cannot see changes occurred in another transaction > > (S'= S+dS) > > in such cases, and we cannot get the incremental change on the view > > (dV=dR*dS). > > Therefore, in the current implementation, the transaction is aborted when > > the > > concurrent view maintenance happens in repeatable read or serializable. > > Sorry, may be I do not correctly understand you or you do not understand me. > Lets consider two serializable transactions (I do not use view or > triggers, but perform correspondent updates manually): > > > > create table t(pk integer, val int); > create table mat_view(gby_key integer primary key, total bigint); > insert into t values (1,0),(2,0); > insert into mat_view values (1,0),(2,0); > > Session 1: Session 2: > > begin isolation level serializable; > begin isolation level serializable; > insert into t values (1,200); insert into t > values (1,300); > update mat_view set total=total+200 where gby_key=1; > update mat_view set total=total+300 where gby_key=1; > > commit; > ERROR: could not serialize access due to concurrent update > > So both transactions are aborted. > It is expected behavior for serializable transactions. > But if transactions updating different records of mat_view, then them > can be executed concurrently: > > Session 1: Session 2: > > begin isolation level serializable; > begin isolation level serializable; > insert into t values (1,200); insert into t > values (2,300); > update mat_view set total=total+200 where gby_key=1; > update mat_view set total=total+300 where gby_key=2; > commit; commit; > > So, if transactions are using serializable isolation level, then we can > update mat view without exclusive lock > and if there is not conflict, this transaction can be executed concurrently. > > Please notice, that exclusive lock doesn't prevent conflict in first case: > > Session 1: Session 2: > > begin isolation level serializable; > begin isolation level serializable; > insert into t values (1,200); insert into t > values (1,300); > lock table mat_view; > update mat_view set total=total+200 where gby_key=1; > lock table mat_view; > > commit; > update mat_view set total=total+300 where gby_key=1; > commit; > ERROR: could not serialize access due to concurrent update > > > So do you agree that there are no reasons for using explicit lock for > serializable transactions? Yes, I agree. I said an anomaly could occur in repeatable read and serializable isolation level, but it was wrong. In serializable, the transaction will be aborted in programmable cases due to predicate locks, and we don't need the lock. However, in repeatable read, the anomaly still could occurs when the table is defined on more than one base tables even if we lock the view. To prevent it, the only way I found is aborting the transaction forcedly in such cases for now. Regards, Yugo Nagata -- Yugo NAGATA
Re: Is Recovery actually paused?
On Thu, 22 Oct 2020 20:36:48 +0530
Dilip Kumar wrote:
> On Thu, Oct 22, 2020 at 7:50 PM Dilip Kumar wrote:
> >
> > On Thu, Oct 22, 2020 at 6:59 AM Kyotaro Horiguchi
> > wrote:
> > >
> > > At Wed, 21 Oct 2020 11:14:24 -0400, Robert Haas
> > > wrote in
> > > > On Wed, Oct 21, 2020 at 7:16 AM Dilip Kumar
> > > > wrote:
> > > > > One idea could be, if the recovery process is waiting for WAL and a
> > > > > recovery pause is requested then we can assume that the recovery is
> > > > > paused because before processing the next wal it will always check
> > > > > whether the recovery pause is requested or not.
> > > ..
> > > > However, it might be better to implement this by having the system
> > > > absorb the pause immediately when it's in this state, rather than
> > > > trying to detect this state and treat it specially.
> > >
> > > The paused state is shown in pg_stat_activity.wait_event and it is
> > > strange that pg_is_wal_replay_paused() is inconsistent with the
> > > column.
> >
> > Right
> >
> > To make them consistent, we need to call recoveryPausesHere()
> > > at the end of WaitForWALToBecomeAvailable() and let
> > > pg_wal_replay_pause() call WakeupRecovery().
> > >
> > > I think we don't need a separate function to find the state.
> >
> > The idea makes sense to me. I will try to change the patch as per the
> > suggestion.
>
> Here is the patch based on this idea.
I reviewd this patch.
First, I made a recovery conflict situation using a table lock.
Standby:
#= begin;
#= select * from t;
Primary:
#= begin;
#= lock t in ;
After this, WAL of the table lock cannot be replayed due to a lock acquired
in the standby.
Second, during the delay, I executed pg_wal_replay_pause() and
pg_is_wal_replay_paused(). Then, pg_is_wal_replay_paused was blocked until
max_standby_streaming_delay was expired, and eventually returned true.
I can also see the same behaviour by setting recovery_min_apply_delay.
So, pg_is_wal_replay_paused waits for recovery to get paused and this works
successfully as expected.
However, I wonder users don't expect pg_is_wal_replay_paused to wait.
Especially, if max_standby_streaming_delay is -1, this will be blocked forever,
although this setting may not be usual. In addition, some users may set
recovery_min_apply_delay for a large. If such users call
pg_is_wal_replay_paused,
it could wait for a long time.
At least, I think we need some descriptions on document to explain
pg_is_wal_replay_paused could wait while a time.
Also, how about adding a new boolean argument to pg_is_wal_replay_paused to
control whether this waits for recovery to get paused or not? By setting its
default value to true or false, users can use the old format for calling this
and the backward compatibility can be maintained.
As another comment, while pg_is_wal_replay_paused is blocking, I can not cancel
the query. I think CHECK_FOR_INTERRUPTS() is necessary in the waiting loop.
+ errhint("Recovery control functions can only be executed
during recovery.")));
There are a few tabs at the end of this line.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Implementing Incremental View Maintenance
Hi, Attached is the revised patch (v20) to add support for Incremental Materialized View Maintenance (IVM). In according with Konstantin's suggestion, I made a few optimizations. 1. Creating an index on the matview automatically When creating incremental maintainable materialized view (IMMV)s, a unique index on IMMV is created automatically if possible. If the view definition query has a GROUP BY clause, the index is created on the columns of GROUP BY expressions. Otherwise, if the view contains all primary key attributes of its base tables in the target list, the index is created on these attributes. Also, if the view has DISTINCT, a unique index is created on all columns in the target list. In other cases, no index is created. In all cases, a NOTICE message is output to inform users that an index is created or that an appropriate index is necessary for efficient IVM. 2. Use a weaker lock on the matview if possible If the view has only one base table in this query, RowExclusiveLock is held on the view instead of AccessExclusiveLock, because we don't need to wait other concurrent transaction's result in order to maintain the view in this case. When the same row in the view is affected due to concurrent maintenances, a row level lock will protect it. On Tue, 24 Nov 2020 12:46:57 +0300 Konstantin Knizhnik wrote: > The most obvious optimization is not to use exclusive table lock if view > depends just on one table (contains no joins). > Looks like there are no any anomalies in this case, are there? I confirmed the effect of this optimizations. First, when I performed pgbench (SF=100) without any materialized views, the results is : pgbench test4 -T 300 -c 8 -j 4 latency average = 6.493 ms tps = 1232.146229 (including connections establishing) Next, created a view as below, I performed the same pgbench. CREATE INCREMENTAL MATERIALIZED VIEW mv_ivm2 AS SELECT bid, count(abalance), sum(abalance), avg(abalance) FROM pgbench_accounts GROUP BY bid; The result is here: [the previous version (v19 with exclusive table lock)] - latency average = 77.677 ms - tps = 102.990159 (including connections establishing) [In the latest version (v20 with weaker lock)] - latency average = 17.576 ms - tps = 455.159644 (including connections establishing) There is still substantial overhead, but we can see that the effect of the optimization. Regards, Yugo Nagata -- Yugo NAGATA IVM_patches_v20.tar.gz Description: application/gzip
Re: Implementing Incremental View Maintenance
Hi hackers, I heard the opinion that this patch is too big and hard to review. So, I wander that we should downsize the patch by eliminating some features and leaving other basic features. If there are more opinions this makes it easer for reviewers to look at this patch, I would like do it. If so, we plan to support only selection, projection, inner-join, and some aggregates in the first release and leave sub-queries, outer-join, and CTE supports to the next release. Regards, Yugo Nagata On Tue, 22 Dec 2020 21:51:36 +0900 Yugo NAGATA wrote: > Hi, > > Attached is the revised patch (v20) to add support for Incremental > Materialized View Maintenance (IVM). > > In according with Konstantin's suggestion, I made a few optimizations. > > 1. Creating an index on the matview automatically > > When creating incremental maintainable materialized view (IMMV)s, > a unique index on IMMV is created automatically if possible. > > If the view definition query has a GROUP BY clause, the index is created > on the columns of GROUP BY expressions. Otherwise, if the view contains > all primary key attributes of its base tables in the target list, the index > is created on these attributes. Also, if the view has DISTINCT, > a unique index is created on all columns in the target list. > In other cases, no index is created. > > In all cases, a NOTICE message is output to inform users that an index is > created or that an appropriate index is necessary for efficient IVM. > > 2. Use a weaker lock on the matview if possible > > If the view has only one base table in this query, RowExclusiveLock is > held on the view instead of AccessExclusiveLock, because we don't > need to wait other concurrent transaction's result in order to > maintain the view in this case. When the same row in the view is > affected due to concurrent maintenances, a row level lock will > protect it. > > On Tue, 24 Nov 2020 12:46:57 +0300 > Konstantin Knizhnik wrote: > > > The most obvious optimization is not to use exclusive table lock if view > > depends just on one table (contains no joins). > > Looks like there are no any anomalies in this case, are there? > > I confirmed the effect of this optimizations. > > First, when I performed pgbench (SF=100) without any materialized views, > the results is : > > pgbench test4 -T 300 -c 8 -j 4 > latency average = 6.493 ms > tps = 1232.146229 (including connections establishing) > > Next, created a view as below, I performed the same pgbench. > CREATE INCREMENTAL MATERIALIZED VIEW mv_ivm2 AS > SELECT bid, count(abalance), sum(abalance), avg(abalance) > FROM pgbench_accounts GROUP BY bid; > > The result is here: > > [the previous version (v19 with exclusive table lock)] > - latency average = 77.677 ms > - tps = 102.990159 (including connections establishing) > > [In the latest version (v20 with weaker lock)] > - latency average = 17.576 ms > - tps = 455.159644 (including connections establishing) > > There is still substantial overhead, but we can see that the effect > of the optimization. > > Regards, > Yugo Nagata > > -- > Yugo NAGATA -- Yugo NAGATA
Re: Small fix on query_id_enabled
On Wed, 14 Feb 2024 07:21:54 +0900 Michael Paquier wrote: > On Tue, Feb 13, 2024 at 11:23:47PM +0800, Julien Rouhaud wrote: > > +1! > > Okay, applied as-is, then. Thank you! Regards, Yugo Nagata > -- > Michael -- Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Fri, 1 Sep 2023 15:42:17 +0800
jian he wrote:
I apologize for this late reply.
> I added a new function append_update_set_caluse, and deleted
> functions: {append_set_clause_for_count, append_set_clause_for_sum,
> append_set_clause_for_avg, append_set_clause_for_minmax}
>
> I guess this way is more extensible/generic than yours.
Do you mean that consolidating such functions to a general function
make easier to support a new aggregate function in future? I'm not
convinced completely yet it because your suggestion seems that every
functions' logic are just put into a new function, but providing a
common interface might make a sense a bit.
By the way, when you attach files other than updated patches that
can be applied to master branch, using ".patch" or ".diff" as the
file extension help to avoid to confuse cfbot (for example, like
basedon_v29_matview_c_refactor_update_set_clause.patch.txt).
> src/backend/commands/matview.c
> 2268: /* For tuple deletion */
> maybe "/* For tuple deletion and update*/" is more accurate?
This "deletion" means deletion of tuple from the view rather
than DELETE statement, so I think this is ok.
> Since the apply delta query is quite complex, I feel like adding some
> "if debug then print out the final querybuf.data end if" would be a
> good idea.
Agreed, it would be helpful for debugging. I think it would be good
to add a debug macro that works if DEBUG_IVM is defined rather than
adding GUC like debug_print_..., how about it?
> we add hidden columns somewhere, also to avoid corner cases, so maybe
> somewhere we should assert total attribute number is sane.
The number of hidden columns to be added depends on the view definition
query, so I wonder the Assert condition would be a bit complex. Could
you explain what are you assume about like for example?
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Incremental View Maintenance, take 2
On Mon, 4 Sep 2023 16:48:02 +0800
jian he wrote:
> other ideas based on v29.
>
> src/include/utils/rel.h
> 680: #define RelationIsIVM(relation) ((relation)->rd_rel->relisivm)
> I guess it would be better to add some comments to address the usage.
> Since all peer macros all have some comments.
OK. I will add comments on this macro.
> pg_class change, I guess we need bump CATALOG_VERSION_NO?
CATALOG_VERSION_NO is frequently bumped up when new features are
committed, so including it in the patch causes frequent needs for
rebase during the review of the patch even if no meaningful change
is made. Therefore, I wonder we don't have to included it in the
patch at this time.
> small issue. makeIvmAggColumn and calc_delta need to add an empty
> return statement?
I'm sorry but I could not understand what you suggested, so could
you give me more explanation?
> style issue. in gram.y, "incremental" upper case?
> + CREATE OptNoLog incremental MATERIALIZED VIEW
> create_mv_target AS SelectStmt opt_with_data
This "incremental" is defined as INCREMENTAL or empty, as below.
incremental:INCREMENTAL { $$ = true; }
| /*EMPTY*/ { $$ = false; }
> I don't know how pgident works, do you need to add some keywords to
> src/tools/pgindent/typedefs.list to make indentation work?
I'm not sure typedefs.list should be updated in each patch, because
tools/pgindent/README said that the latest typedef file is downloaded
from the buildfarm when pgindent is run.
> in
> /* If this is not the last AFTER trigger call, immediately exit. */
> Assert (entry->before_trig_count >= entry->after_trig_count);
> if (entry->before_trig_count != entry->after_trig_count)
> return PointerGetDatum(NULL);
>
> before returning NULL, do you also need clean_up_IVM_hash_entry? (I
> don't know when this case will happen)
No, clean_up_IVM_hash_entry is not necessary in this case.
When multiple tables are updated in a statement, statement-level AFTER
triggers collects every information of the tables, and the last AFTER
trigger have to perform the actual maintenance of the view. To make sure
this, the number that BEFORE and AFTER trigger is fired is counted
respectively, and when they match it is regarded the last AFTER trigger
call performing the maintenance. Until this, collected information have
to keep, so we cannot call clean_up_IVM_hash_entry.
> in
> /* Replace the modified table with the new delta table and
> calculate the new view delta*/
> replace_rte_with_delta(rte, table, true, queryEnv);
> refresh_matview_datafill(dest_new, query, queryEnv, tupdesc_new, "");
>
> replace_rte_with_delta does not change the argument: table, argument:
> queryEnv. refresh_matview_datafill just uses the partial argument of
> the function calc_delta. So I guess, I am confused by the usage of
> replace_rte_with_delta. also I think it should return void, since you
> just modify the input argument. Here refresh_matview_datafill is just
> persisting new delta content to dest_new?
Yes, refresh_matview_datafill executes the query and the result rows to
"dest_new". And, replace_rte_with_delta updates the input argument "rte"
and returns the result to it, so it may be better that this returns void,
as you suggested.
Regards,
Yugo Nagata
--
Yugo NAGATA
Remove unnecessary code from psql's watch command
Hi,
In the current code of do_watch(), sigsetjmp is called if WIN32
is defined, but siglongjmp is not called in the signal handler
in this condition. On Windows, currently, cancellation is checked
only by cancel_pressed, and calling sigsetjmp in do_watch() is
unnecessary. Therefore, we can remove code around sigsetjmp in
do_watch(). I've attached the patch for this fix.
Regards,
Yugo Ngata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..c03e47744e 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,20 +5312,10 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
@@ -5335,7 +5325,6 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
break;
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, &sigint, NULL);
Re: Remove unnecessary code from psql's watch command
On Wed, 06 Mar 2024 13:03:39 -0500
Tom Lane wrote:
> Michael Paquier writes:
> > On Tue, Mar 05, 2024 at 10:05:52PM +0900, Yugo NAGATA wrote:
> >> In the current code of do_watch(), sigsetjmp is called if WIN32
> >> is defined, but siglongjmp is not called in the signal handler
> >> in this condition. On Windows, currently, cancellation is checked
> >> only by cancel_pressed, and calling sigsetjmp in do_watch() is
> >> unnecessary. Therefore, we can remove code around sigsetjmp in
> >> do_watch(). I've attached the patch for this fix.
>
> > Re-reading the top comment of sigint_interrupt_enabled, it looks like
> > you're right here. As long as we check for cancel_pressed there
> > should be no need for any special cancellation handling here.
>
> I don't have Windows here to test on, but does the WIN32 code
> path work at all? It looks to me like cancel_pressed becoming
> true doesn't get us to exit the outer loop, only the inner delay
> one, meaning that trying to control-C out of a \watch will just
> cause it to repeat the command even faster. That path isn't
> setting the "done" variable, and it isn't checking it either,
> because all of that only happens in the other #ifdef arm.
The outer loop is eventually exited even because PSQLexecWatch returns 0
when cancel_pressed = 0. However, it happens after executing an extra
query in this function not just after exit of the inner loop. Therefore,
it would be better to adding set and check of "done" in WIN32, too.
I've attached the updated patch
(v2_remove_unnecessary_code_in_psql_watch.patch).
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index 5c906e4806..6827d8b8d0 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -5312,30 +5312,22 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
continue;
#ifdef WIN32
-
- /*
- * Set up cancellation of 'watch' via SIGINT. We redo this each time
- * through the loop since it's conceivable something inside
- * PSQLexecWatch could change sigint_interrupt_jmp.
- */
- if (sigsetjmp(sigint_interrupt_jmp, 1) != 0)
- break;
-
/*
- * Enable 'watch' cancellations and wait a while before running the
- * query again. Break the sleep into short intervals (at most 1s).
+ * Wait a while before running the query again. Break the sleep into
+ * short intervals (at most 1s).
*/
- sigint_interrupt_enabled = true;
for (long i = sleep_ms; i > 0;)
{
long s = Min(i, 1000L);
pg_usleep(s * 1000L);
if (cancel_pressed)
+ {
+done = true;
break;
+ }
i -= s;
}
- sigint_interrupt_enabled = false;
#else
/* sigwait() will handle SIGINT. */
sigprocmask(SIG_BLOCK, &sigint, NULL);
@@ -5369,9 +5361,9 @@ do_watch(PQExpBuffer query_buf, double sleep, int iter, int min_rows)
/* Unblock SIGINT so that slow queries can be interrupted. */
sigprocmask(SIG_UNBLOCK, &sigint, NULL);
+#endif
if (done)
break;
-#endif
}
if (pagerpipe)
Fix cancellation check in ExecQueryAndProcessResults
Hi,
While looking ExecQueryAndProcessResults, I found the following code.
/*
* If SIGINT is sent while the query is processing, the interrupt will be
* consumed. The user's intention, though, is to cancel the entire watch
* process, so detect a sent cancellation request and exit in this case.
*/
if (is_watch && cancel_pressed)
{
ClearOrSaveAllResults();
return 0;
}
I guess the intention is that when the query is cancelled during \watch process,
we want to exit early before handling the error. However, we cannot detect the
cancel at this timing because currently we use PQsendQuery which is asynchronous
and does not wait the result. We have to check cancel_pressed after PQgetResult
call. I'm also attached a patch for this, with some comments fix.
Regards,
Yugo Nagata
--
Yugo NAGATA
diff --git a/src/bin/psql/common.c b/src/bin/psql/common.c
index 76e01b02a3..7b83e221fd 100644
--- a/src/bin/psql/common.c
+++ b/src/bin/psql/common.c
@@ -1406,9 +1406,9 @@ DescribeQuery(const char *query, double *elapsed_msec)
* For other commands, the results are processed normally, depending on their
* status.
*
- * Returns 1 on complete success, 0 on interrupt and -1 or errors. Possible
- * failure modes include purely client-side problems; check the transaction
- * status for the server-side opinion.
+ * Returns 1 on complete success, 0 on interrupt or results less than min_rows
+ * rows, and -1 on errors. Possible failure modes include purely client-side
+ * problems; check the transaction status for the server-side opinion.
*
* Note that on a combined query, failure does not mean that nothing was
* committed.
@@ -1450,6 +1450,9 @@ ExecQueryAndProcessResults(const char *query,
return -1;
}
+ /* first result */
+ result = PQgetResult(pset.db);
+
/*
* If SIGINT is sent while the query is processing, the interrupt will be
* consumed. The user's intention, though, is to cancel the entire watch
@@ -1461,8 +1464,6 @@ ExecQueryAndProcessResults(const char *query,
return 0;
}
- /* first result */
- result = PQgetResult(pset.db);
if (min_rows > 0 && PQntuples(result) < min_rows)
{
return_early = true;
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 7 Mar 2024 16:56:17 -0600
Nathan Bossart wrote:
> On Mon, Feb 05, 2024 at 04:28:23PM +0900, Yugo NAGATA wrote:
> > On Thu, 1 Feb 2024 17:59:56 +0800
> > jian he wrote:
> >> v6 patch looks good.
> >
> > Thank you for your review and updating the status to RwC!
>
> I think this one needs a (pretty trivial) rebase. I spent a few minutes
> testing it out and looking at the code, and it seems generally reasonable
Thank you for your review.
I've attached a rebased patch.
> to me. Do you think it's worth adding something like a
> pg_column_toast_num_chunks() function that returns the number of chunks for
> the TOASTed value, too?
If we want to know the number of chunks of a specified chunk_id,
we can get this by the following query.
postgres=# SELECT id, (SELECT count(*) FROM pg_toast.pg_toast_16384 WHERE
chunk_id = id)
FROM (SELECT pg_column_toast_chunk_id(v) AS id FROM t);
id | count
---+---
16389 | 3
16390 | 287
(2 rows)
However, if there are needs for getting such information in a
simpler way, it might be worth making a new function.
Regards,
Yugo Nagata
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From 7cd0a1cbeae11dea4f4020c71cd0a4550e17b26b Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v7] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 291ed876fc..443ca854a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7447,6 +7447,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text',
proargtypes => 'any', prosrc => 'pg_column_compression' },
+{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value&#x
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Fri, 8 Mar 2024 16:17:58 -0600
Nathan Bossart wrote:
> On Fri, Mar 08, 2024 at 03:31:55PM +0900, Yugo NAGATA wrote:
> > On Thu, 7 Mar 2024 16:56:17 -0600
> > Nathan Bossart wrote:
> >> to me. Do you think it's worth adding something like a
> >> pg_column_toast_num_chunks() function that returns the number of chunks for
> >> the TOASTed value, too?
> >
> > If we want to know the number of chunks of a specified chunk_id,
> > we can get this by the following query.
> >
> > postgres=# SELECT id, (SELECT count(*) FROM pg_toast.pg_toast_16384 WHERE
> > chunk_id = id)
> > FROM (SELECT pg_column_toast_chunk_id(v) AS id FROM t);
>
> Good point. Overall, I think this patch is in decent shape, so I'll aim to
> commit it sometime next week.
Thank you.
>
> > +{ oid => '8393', descr => 'chunk ID of on-disk TOASTed value',
> > + proname => 'pg_column_toast_chunk_id', provolatile => 's', prorettype =>
> > 'oid',
> > + proargtypes => 'any', prosrc => 'pg_column_toast_chunk_id' },
>
> Note to self: this change requires a catversion bump.
>
> > +INSERT INTO test_chunk_id(v1,v2)
> > + VALUES (repeat('x', 1), repeat('x', 2048));
>
> Is this guaranteed to be TOASTed for all possible page sizes?
Should we use block_size?
SHOW block_size \gset
INSERT INTO test_chunk_id(v1,v2)
VALUES (repeat('x', 1), repeat('x', (:block_size / 4)));
I think this will work in various page sizes.
I've attached a patch in which the test is updated.
Regards,
Yugo Nagata
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From c3b99418d1ae3a8235db9bedd95e7d6ac1556f90 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v8] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 27 +
src/test/regress/sql/misc_functions.sql | 24
5 files changed, 112 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff
Re: Remove unnecessary code from psql's watch command
On Fri, 08 Mar 2024 12:09:12 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > On Wed, 06 Mar 2024 13:03:39 -0500 > > Tom Lane wrote: > >> I don't have Windows here to test on, but does the WIN32 code > >> path work at all? > > > The outer loop is eventually exited even because PSQLexecWatch returns 0 > > when cancel_pressed = 0. However, it happens after executing an extra > > query in this function not just after exit of the inner loop. Therefore, > > it would be better to adding set and check of "done" in WIN32, too. > > Ah, I see now. Agreed, that could stand improvement. > > > I've attached the updated patch > > (v2_remove_unnecessary_code_in_psql_watch.patch). > > Pushed with minor tidying. Thanks! -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Sat, 9 Mar 2024 08:50:28 -0600
Nathan Bossart wrote:
> On Sat, Mar 09, 2024 at 11:57:18AM +0900, Yugo NAGATA wrote:
> > On Fri, 8 Mar 2024 16:17:58 -0600
> > Nathan Bossart wrote:
> >> Is this guaranteed to be TOASTed for all possible page sizes?
> >
> > Should we use block_size?
> >
> > SHOW block_size \gset
> > INSERT INTO test_chunk_id(v1,v2)
> > VALUES (repeat('x', 1), repeat('x', (:block_size / 4)));
> >
> > I think this will work in various page sizes.
>
> WFM
>
> > +SHOW block_size; \gset
> > + block_size
> > +
> > + 8192
> > +(1 row)
>
> I think we need to remove the ';' so that the output of the query is not
> saved in the ".out" file. With that change, this test passes when Postgres
> is built with --with-blocksize=32. However, many other unrelated tests
> begin failing, so I guess this fix isn't tremendously important.
I rewrote the patch to use current_setting('block_size') instead of SHOW
and \gset as other tests do. Although some tests are failing with block_size=32,
I wonder it is a bit better to use "block_size" instead of the constant
to make the test more general to some extent.
Regards,
Yugo Nagata
>
> --
> Nathan Bossart
> Amazon Web Services: https://aws.amazon.com
--
Yugo NAGATA
>From 5b3be1ca6f8d8bafc1d9ce7bea252f364c9c09a9 Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Wed, 29 Mar 2023 09:59:25 +0900
Subject: [PATCH v9] Add pg_column_toast_chunk_id function
This function returns the chunk_id of an on-disk TOASTed value, or
NULL if the value is un-TOASTed or not on disk. This enables users to
know which columns are actually TOASTed. This function is also useful
to identify a problematic row when an error like
"ERROR: unexpected chunk number ... (expected ...) for toast value"
occurs.
---
doc/src/sgml/func.sgml | 17
src/backend/utils/adt/varlena.c | 41
src/include/catalog/pg_proc.dat | 3 ++
src/test/regress/expected/misc_functions.out | 21 ++
src/test/regress/sql/misc_functions.sql | 23 +++
5 files changed, 105 insertions(+)
diff --git a/doc/src/sgml/func.sgml b/doc/src/sgml/func.sgml
index 0bb7aeb40e..3169e2aeb7 100644
--- a/doc/src/sgml/func.sgml
+++ b/doc/src/sgml/func.sgml
@@ -28552,6 +28552,23 @@ postgres=# SELECT '0/0'::pg_lsn + pd.segment_number * ps.setting::int + :offset
+
+
+
+ pg_column_toast_chunk_id
+
+pg_column_toast_chunk_id ( "any" )
+oid
+
+
+Shows the chunk_id of an on-disk
+TOASTed value. Returns NULL
+if the value is un-TOASTed or not on-disk.
+See for details about
+TOAST.
+
+
+
diff --git a/src/backend/utils/adt/varlena.c b/src/backend/utils/adt/varlena.c
index 543afb66e5..84d36781a4 100644
--- a/src/backend/utils/adt/varlena.c
+++ b/src/backend/utils/adt/varlena.c
@@ -5105,6 +5105,47 @@ pg_column_compression(PG_FUNCTION_ARGS)
PG_RETURN_TEXT_P(cstring_to_text(result));
}
+/*
+ * Return the chunk_id of the on-disk TOASTed value.
+ * Return NULL if the value is unTOASTed or not on disk.
+ */
+Datum
+pg_column_toast_chunk_id(PG_FUNCTION_ARGS)
+{
+ int typlen;
+ struct varlena *attr;
+ struct varatt_external toast_pointer;
+
+ /* On first call, get the input type's typlen, and save at *fn_extra */
+ if (fcinfo->flinfo->fn_extra == NULL)
+ {
+ /* Lookup the datatype of the supplied argument */
+ Oid argtypeid = get_fn_expr_argtype(fcinfo->flinfo, 0);
+
+ typlen = get_typlen(argtypeid);
+ if (typlen == 0) /* should not happen */
+ elog(ERROR, "cache lookup failed for type %u", argtypeid);
+
+ fcinfo->flinfo->fn_extra = MemoryContextAlloc(fcinfo->flinfo->fn_mcxt,
+ sizeof(int));
+ *((int *) fcinfo->flinfo->fn_extra) = typlen;
+ }
+ else
+ typlen = *((int *) fcinfo->flinfo->fn_extra);
+
+ if (typlen != -1)
+ PG_RETURN_NULL();
+
+ attr = (struct varlena *) DatumGetPointer(PG_GETARG_DATUM(0));
+
+ if (!VARATT_IS_EXTERNAL_ONDISK(attr))
+ PG_RETURN_NULL();
+
+ VARATT_EXTERNAL_GET_POINTER(toast_pointer, attr);
+
+ PG_RETURN_OID(toast_pointer.va_valueid);
+}
+
/*
* string_agg - Concatenates values and returns string.
*
diff --git a/src/include/catalog/pg_proc.dat b/src/include/catalog/pg_proc.dat
index 291ed876fc..443ca854a6 100644
--- a/src/include/catalog/pg_proc.dat
+++ b/src/include/catalog/pg_proc.dat
@@ -7447,6 +7447,9 @@
{ oid => '2121', descr => 'compression method for the compressed datum',
proname => 'pg_column_compression', provolatile => 's', prorettype => 'text
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Tue, 12 Mar 2024 22:07:17 -0500 Nathan Bossart wrote: > I did some light editing to prepare this for commit. Thank you. I confirmed the test you improved and I am fine with that. Regards, Yugo Nagata > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Re: pg_column_toast_chunk_id: a function to get a chunk ID of a TOASTed value
On Thu, 14 Mar 2024 11:10:42 -0500 Nathan Bossart wrote: > On Wed, Mar 13, 2024 at 01:09:18PM +0700, Yugo NAGATA wrote: > > On Tue, 12 Mar 2024 22:07:17 -0500 > > Nathan Bossart wrote: > >> I did some light editing to prepare this for commit. > > > > Thank you. I confirmed the test you improved and I am fine with that. > > Committed. Thank you! > > -- > Nathan Bossart > Amazon Web Services: https://aws.amazon.com -- Yugo NAGATA
Allow an extention to be updated without a script
Hi hackers,
I propose to add a new option "updates_without_script" to extension's
control file which a list of updates that do not need update script.
This enables to update an extension by ALTER EXTENSION even if the
extension module doesn't provide the update script.
Currently, even when we don't need to execute any command to update an
extension from one version to the next, we need to provide an update
script that doesn't contain any command. Preparing such meaningless
files are sometimes annoying.
The attached patch introduces a new option "updates_without_script"
into extension control file. This specifies a list of such updates
following the pattern 'old_version--target_version'.
For example,
updates_without_script = '1.1--1.2, 1.3--1.4'
means that updates from version 1.1 to version 1.2 and from version 1.3
to version 1.4 don't need an update script. In this case, users don't
need to prepare update scripts extension--1.1--1.2.sql and
extension--1.3--1.4.sql if it is not necessary to execute any commands.
The updated path of an extension is determined based on both the filenames
of update scripts and the list of updates specified in updates_without_script.
Presence of update script has higher priority than the option. Therefore,
if an update script is provided, the script will be executed even if this
update is specified in updates_without_script.
What do you think of this feature?
Any feedback would be appreciated.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From 938e15b28f66015044b559e4c523fc74590691fc Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Mon, 30 Jan 2023 17:36:55 +0900
Subject: [PATCH] Allow an extention to be updated without a script
When we don't need to execute any command to update an extension from one
version to the next, we can specify a list of such updates following
the pattern 'old_version--target_version' into a new option
updates_without_script. For example, specifying '1.1--1.2, 1.3--1.4'
means updates from version 1.1 to version 1.2, and from version 1.3
to version 1.4 don't need an update script. User doesn't need to
provide an update script that doesn't contain any command for such
updates.
The updated path is determined based on both the names of update scripts
and the list in updates_without_script. If an update script is provided,
the script will be executed even if this update is specified in
updates_without_script.
---
doc/src/sgml/extend.sgml | 30 +++-
src/backend/commands/extension.c | 161 +++---
src/test/modules/test_extensions/Makefile | 3 +-
.../expected/test_extensions.out | 6 +
.../test_extensions/sql/test_extensions.sql | 8 +
.../test_extensions/test_ext9--1.0.sql| 0
.../test_extensions/test_ext9--2.0--3.0.sql | 0
.../modules/test_extensions/test_ext9.control | 5 +
8 files changed, 182 insertions(+), 31 deletions(-)
create mode 100644 src/test/modules/test_extensions/test_ext9--1.0.sql
create mode 100644 src/test/modules/test_extensions/test_ext9--2.0--3.0.sql
create mode 100644 src/test/modules/test_extensions/test_ext9.control
diff --git a/doc/src/sgml/extend.sgml b/doc/src/sgml/extend.sgml
index 46e873a166..1c4c978264 100644
--- a/doc/src/sgml/extend.sgml
+++ b/doc/src/sgml/extend.sgml
@@ -807,6 +807,17 @@ RETURNS anycompatible AS ...
+
+
+ updates_without_script (string)
+
+
+A list of updates that do not need update scripts following the pattern
+old_version--target_version,
+for example updates_without_script = '1.1--1.2, 1.3--1.4'.
+
+
+
@@ -818,8 +829,9 @@ RETURNS anycompatible AS ...
Secondary control files follow the same format as the primary control
file. Any parameters set in a secondary control file override the
primary control file when installing or updating to that version of
- the extension. However, the parameters directory and
- default_version cannot be set in a secondary control file.
+ the extension. However, the parameters directory,
+ default_version, and updates_without_script
+ cannot be set in a secondary control file.
@@ -1092,6 +1104,20 @@ SELECT pg_catalog.pg_extension_config_dump('my_config', 'WHERE NOT standard_entr
objects, they are automatically dissociated from the extension.
+
+ If you don't need to execute any command to update an extension from one
+ version to the next, provide an update script that doesn't contain
+ any command or specify a list of such updates following the pattern
+ old_version--target_version
+ into updates_without_script. For example,
+ updates_without_script = '1.1--1.2, 1.3--1.4'
+ means updates from version 1.1 to ver
Re: Allow an extention to be updated without a script
Hi, Thank you for your comment. On Mon, 30 Jan 2023 16:05:52 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > Currently, even when we don't need to execute any command to update an > > extension from one version to the next, we need to provide an update > > script that doesn't contain any command. Preparing such meaningless > > files are sometimes annoying. > > If you have no update script, why call it a new version? The point > of extension versions is to distinguish different states of the > extension's SQL objects. We do not consider mods in underlying C code > to justify a new version. Although, as you say, the extension manager doesn't track changes in C code functions, a new version could be released with changes in only in C functions that implement a new feature or fix some bugs because it looks a new version from user's view. Actually, there are several extensions that provide empty update scripts in order to allow user to install such new versions, for example; - pglogical (https://github.com/2ndQuadrant/pglogical/blob/REL2_x_STABLE/pglogical--2.4.1--2.4.2.sql) - hll (https://github.com/citusdata/postgresql-hll/blob/master/update/hll--2.16--2.17.sql) - orafce (https://github.com/orafce/orafce/blob/master/orafce--3.12--3.13.sql) - hypopg (https://github.com/HypoPG/hypopg/blob/REL1_STABLE/hypopg--1.3.1--1.3.2.sql) - timescaledb (https://github.com/timescale/timescaledb/blob/main/sql/updates/2.9.2--2.9.1.sql) -- Yugo NAGATA
make_ctags: use -I option to ignore pg_node_attr macro
Hi, I found that tag files generated by src/tools/make_ctags doesn't include some keywords, that were field names of node structs, for example norm_select in RestrictInfo. Such fields are defined with pg_node_attr macro that was introduced recently, like: Selectivity norm_selec pg_node_attr(equal_ignore); In this case, pg_node_attr is mistakenly interpreted to be the name of the field. So, I propose to use -I option of ctags to ignore the marco name. Attached is a patch to do it. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/tools/make_ctags b/src/tools/make_ctags index 912b6fafac..32b475e82c 100755 --- a/src/tools/make_ctags +++ b/src/tools/make_ctags @@ -34,9 +34,12 @@ then FLAGS="--c-kinds=+dfmstuv" else FLAGS="-dt" fi +# List of identifiers to ignore +IGNORE_LIST="pg_node_attr+" + # this is outputting the tags into the file 'tags', and appending find `pwd`/ -type f -name '*.[chyl]' -print | - xargs ctags -a -f tags "$FLAGS" + xargs ctags -a -f tags "$FLAGS" -I "$IGNORE_LIST" # Exuberant tags has a header that we cannot sort in with the other entries # so we skip the sort step
Re: make_ctags: use -I option to ignore pg_node_attr macro
Hello On Mon, 10 Oct 2022 12:04:22 +0200 Alvaro Herrera wrote: > Hello > > On 2022-Oct-07, Yugo NAGATA wrote: > > > I found that tag files generated by src/tools/make_ctags > > doesn't include some keywords, that were field names of node > > structs, for example norm_select in RestrictInfo. Such fields > > are defined with pg_node_attr macro that was introduced > > recently, like: > > > > Selectivity norm_selec pg_node_attr(equal_ignore); > > > > In this case, pg_node_attr is mistakenly interpreted to be > > the name of the field. So, I propose to use -I option of ctags > > to ignore the marco name. Attached is a patch to do it. > > I've wondered if there's anybody that uses this script. I suppose if > you're reporting this problem then it has at least one user, and > therefore worth fixing. Yeah, I am a make_ctags user, there may be few users though > If we do patch it, how about doing some more invasive surgery and > bringing it to the XXI century? I think the `find` line is a bit lame: > > > # this is outputting the tags into the file 'tags', and appending > > find `pwd`/ -type f -name '*.[chyl]' -print | > > - xargs ctags -a -f tags "$FLAGS" > > + xargs ctags -a -f tags "$FLAGS" -I "$IGNORE_LIST" > > especially because it includes everything in tmp_install, which pollutes > the tag list. > > In my own tags script I just call "ctags -R", and I feed cscope with > these find lines > > (find $SRCDIR \( -name tmp_install -prune -o -name tmp_check -prune \) -o \( > -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" -o > -name "*.sh" -o -name "*.sgml" -o -name "*.sql" -o -name "*.p[lm]" \) -type f > -print ; \ > find $BUILDDIR \( -name tmp_install -prune \) -o \( -name \*.h -a -type f \) > -print ) Thank you for your comments. I updated the patch to ignore the code under tmp_install and add some file types like sql, p[lm], and so on. .sgml or .sh is not included because they don't seem to be beneficial for ctags. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/src/tools/make_ctags b/src/tools/make_ctags index 912b6fafac..b1070dcea9 100755 --- a/src/tools/make_ctags +++ b/src/tools/make_ctags @@ -34,9 +34,17 @@ then FLAGS="--c-kinds=+dfmstuv" else FLAGS="-dt" fi +# Use -I option to ignore a macro +if [ "$IS_EXUBERANT" ] +then IGNORE_IDENTIFIES="-I pg_node_attr+" +else IGNORE_IDENTIFIES= +fi + # this is outputting the tags into the file 'tags', and appending -find `pwd`/ -type f -name '*.[chyl]' -print | - xargs ctags -a -f tags "$FLAGS" +find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \ + -o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" \ + -o -name "*.sql" -o -name "*.p[lm]" \) -type f -print | + xargs ctags -a -f tags "$FLAGS" "$IGNORE_IDENTIFIES" # Exuberant tags has a header that we cannot sort in with the other entries # so we skip the sort step
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Thu, 13 Oct 2022 15:35:09 +0900 (JST)
Tatsuo Ishii wrote:
> > OK, that sounds good then. I would make a feature request to have a
> > switch that supresses creation of these links, then.
>
> Ok, I have added "-n" option to make_ctags so that it skips to create
> the links.
>
> Also I have changed make_etags so that it exec make_ctags, which seems
> to be the consensus.
Thank you for following up my patch.
I fixed the patch to allow use both -e and -n options together.
Regards,
Yugo Nagata
>
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA
diff --git a/src/tools/make_ctags b/src/tools/make_ctags
index 912b6fafac..8062ff2d3e 100755
--- a/src/tools/make_ctags
+++ b/src/tools/make_ctags
@@ -1,12 +1,37 @@
#!/bin/sh
-# src/tools/make_ctags
+# src/tools/make_ctags [-e] [-n]
+# If -e is specified, generate tags files for emacs.
+# If -n is specified, don't create symbolic links of tags file.
+usage="$0 [-e][-n]"
+if [ $# -gt 2 ];then
+echo $usage
+exit 1
+fi
+
+mode=
+no_symlink=
+tags_file=tags
+
+while [ $# -gt 0 ]
+do
+if [ $1 = "-e" ];then
+ mode="-e"
+ tags_file=TAGS
+elif [ $1 = "-n" ];then
+ no_symlink=yes
+else
+ echo $usage
+ exit 1
+fi
+shift
+done
command -v ctags >/dev/null || \
{ echo "'ctags' program not found" 1>&2; exit 1; }
trap "ret=$?; rm -rf /tmp/$$; exit $ret" 0 1 2 3 15
-rm -f ./tags
+rm -f ./$tags_file
IS_EXUBERANT=""
ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
@@ -34,9 +59,17 @@ then FLAGS="--c-kinds=+dfmstuv"
else FLAGS="-dt"
fi
+# Use -I option to ignore a macro
+if [ "$IS_EXUBERANT" ]
+then IGNORE_IDENTIFIES="-I pg_node_attr+"
+else IGNORE_IDENTIFIES=
+fi
+
# this is outputting the tags into the file 'tags', and appending
-find `pwd`/ -type f -name '*.[chyl]' -print |
- xargs ctags -a -f tags "$FLAGS"
+find `pwd`/ \( -name tmp_install -prune -o -name tmp_check -prune \) \
+ -o \( -name "*.[chly]" -o -iname "*makefile*" -o -name "*.mk" -o -name "*.in" \
+ -o -name "*.sql" -o -name "*.p[lm]" \) -type f -print |
+ xargs ctags $mode -a -f $tags_file "$FLAGS" "$IGNORE_IDENTIFIES"
# Exuberant tags has a header that we cannot sort in with the other entries
# so we skip the sort step
@@ -45,10 +78,13 @@ find `pwd`/ -type f -name '*.[chyl]' -print |
if [ ! "$IS_EXUBERANT" ]
then LC_ALL=C
export LC_ALL
- sort tags >/tmp/$$ && mv /tmp/$$ tags
+ sort $tags_file >/tmp/$$ && mv /tmp/$$ $tags_file
fi
-find . \( -name 'CVS' -prune \) -o \( -name .git -prune \) -o -type d -print |
-while read DIR
-do [ "$DIR" != "." ] && ln -f -s `echo "$DIR" | sed 's;/[^/]*;/..;g'`/tags "$DIR"/tags
-done
+# create symblink links
+if [ ! "$no_symlink" ];then
+find . \( -name 'CVS' -prune \) -o \( -name .git -prune \) -o -type d -print |
+ while read DIR
+ do [ "$DIR" != "." ] && ln -f -s `echo "$DIR" | sed 's;/[^/]*;/..;g'`/$tags_file "$DIR"/$tags_file
+ done
+fi
diff --git a/src/tools/make_etags b/src/tools/make_etags
index 9288ef7b14..afc57e3e89 100755
--- a/src/tools/make_etags
+++ b/src/tools/make_etags
@@ -1,16 +1,6 @@
#!/bin/sh
-
# src/tools/make_etags
-command -v etags >/dev/null || \
- { echo "'etags' program not found" 1>&2; exit 1; }
-
-rm -f ./TAGS
-
-find `pwd`/ -type f -name '*.[chyl]' -print |
- xargs etags --append -o TAGS
-
-find . \( -name CVS -prune \) -o \( -name .git -prune \) -o -type d -print |
-while read DIR
-do [ "$DIR" != "." ] && ln -f -s `pwd`/TAGS "$DIR"
-done
+cdir=`dirname $0`
+dir=`(cd $cdir && pwd)`
+exec $dir/make_ctags -e $*
Re: SI-read predicate locks on materialized views
Hello Micheal-san,
On Thu, 13 Oct 2022 17:02:06 +0900
Michael Paquier wrote:
> On Fri, Sep 30, 2022 at 10:12:13AM +0900, Yugo NAGATA wrote:
> > Thank you for comment. Do you think it can be marked as Ready for Commiter?
>
> Matviews have been discarded from needing predicate locks since
> 3bf3ab8 and their introduction, where there was no concurrent flavor
> of refresh yet. Shouldn't this patch have at least an isolation test
> to show the difference in terms of read-write conflicts with some
> serializable transactions and REFRESH CONCURRENTLY?
Thank you for your review. I agree that an isolation test is required.
The attached patch contains the test using the scenario as explained in
the previous post.
Regards,
Yugo Nagata
--
Yugo NAGATA
>From b60a53a945283de4b068e1bc7aaafec26dd8f4da Mon Sep 17 00:00:00 2001
From: Yugo Nagata
Date: Tue, 18 Oct 2022 17:15:33 +0900
Subject: [PATCH] SI-read predicate locking on materialized views
Matviews have been discarded from needing predicate locks since
3bf3ab8 and their introduction, where there was no concurrent flavor
of refresh yet.
---
src/backend/storage/lmgr/predicate.c | 5 +-
.../isolation/expected/matview-write-skew.out | 77 +++
src/test/isolation/isolation_schedule | 1 +
.../isolation/specs/matview-write-skew.spec | 43 +++
4 files changed, 123 insertions(+), 3 deletions(-)
create mode 100644 src/test/isolation/expected/matview-write-skew.out
create mode 100644 src/test/isolation/specs/matview-write-skew.spec
diff --git a/src/backend/storage/lmgr/predicate.c b/src/backend/storage/lmgr/predicate.c
index e8120174d6..df1c0d72e9 100644
--- a/src/backend/storage/lmgr/predicate.c
+++ b/src/backend/storage/lmgr/predicate.c
@@ -490,14 +490,13 @@ static void ReleasePredicateLocksLocal(void);
/*
* Does this relation participate in predicate locking? Temporary and system
- * relations are exempt, as are materialized views.
+ * relations are exempt.
*/
static inline bool
PredicateLockingNeededForRelation(Relation relation)
{
return !(relation->rd_id < FirstUnpinnedObjectId ||
- RelationUsesLocalBuffers(relation) ||
- relation->rd_rel->relkind == RELKIND_MATVIEW);
+ RelationUsesLocalBuffers(relation));
}
/*
diff --git a/src/test/isolation/expected/matview-write-skew.out b/src/test/isolation/expected/matview-write-skew.out
new file mode 100644
index 00..c1f8b3f5ea
--- /dev/null
+++ b/src/test/isolation/expected/matview-write-skew.out
@@ -0,0 +1,77 @@
+Parsed test spec with 2 sessions
+
+starting permutation: rxwy1 c1 rywx2 c2
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step c1: COMMIT;
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step c2: COMMIT;
+
+starting permutation: rxwy1 rywx2 c1 c2
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step c1: COMMIT;
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+
+starting permutation: rxwy1 rywx2 c2 c1
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step c2: COMMIT;
+step c1: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+
+starting permutation: rywx2 rxwy1 c1 c2
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step c1: COMMIT;
+step c2: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+
+starting permutation: rywx2 rxwy1 c2 c1
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step c2: COMMIT;
+step c1: COMMIT;
+ERROR: could not serialize access due to read/write dependencies among transactions
+
+starting permutation: rywx2 c2 rxwy1 c1
+step rywx2:
+ DO $$DECLARE today date;
+ BEGIN
+SELECT INTO today max(date) + 1 FROM order_summary;
+INSERT INTO orders VALUES (today, 'banana', 10);
+ END$$;
+
+step c2: COMMIT;
+step rxwy1: REFRESH MATERIALIZED VIEW CONCURRENTLY order_summary;
+step c1: COMMIT;
diff --git a/src/test/isolation/isolation_schedule b/src/test/i
Re: make_ctags: use -I option to ignore pg_node_attr macro
Hi, On Sat, 15 Oct 2022 10:40:29 +0900 (JST) Tatsuo Ishii wrote: > > On Thu, 13 Oct 2022 15:35:09 +0900 (JST) > > Tatsuo Ishii wrote: > > > >> > OK, that sounds good then. I would make a feature request to have a > >> > switch that supresses creation of these links, then. > >> > >> Ok, I have added "-n" option to make_ctags so that it skips to create > >> the links. > >> > >> Also I have changed make_etags so that it exec make_ctags, which seems > >> to be the consensus. > > > > Thank you for following up my patch. > > I fixed the patch to allow use both -e and -n options together. > > Thanks. I have made mostly cosmetic changes so that it is more > consistent with existing scripts. > > I would like to push v6 patch if there's no objection. I am fine with this patch. By the way, in passing, how about adding "tags" and "TAGS" to .gitignore file? > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Wed, 19 Oct 2022 13:25:17 +0900 (JST) Tatsuo Ishii wrote: > > Hi, > > > > On Sat, 15 Oct 2022 10:40:29 +0900 (JST) > > Tatsuo Ishii wrote: > > > >> > On Thu, 13 Oct 2022 15:35:09 +0900 (JST) > >> > Tatsuo Ishii wrote: > >> > > >> >> > OK, that sounds good then. I would make a feature request to have a > >> >> > switch that supresses creation of these links, then. > >> >> > >> >> Ok, I have added "-n" option to make_ctags so that it skips to create > >> >> the links. > >> >> > >> >> Also I have changed make_etags so that it exec make_ctags, which seems > >> >> to be the consensus. > >> > > >> > Thank you for following up my patch. > >> > I fixed the patch to allow use both -e and -n options together. > >> > >> Thanks. I have made mostly cosmetic changes so that it is more > >> consistent with existing scripts. > >> > >> I would like to push v6 patch if there's no objection. > > > > I am fine with this patch. > > Thanks. the v6 patch pushed to master branch. Thanks! > > By the way, in passing, how about adding "tags" and "TAGS" to > > .gitignore file? > > Sounds like a good idea. Ok, the patch is attached. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/.gitignore b/.gitignore index 794e35b73c..ca1413427b 100644 --- a/.gitignore +++ b/.gitignore @@ -31,6 +31,8 @@ win32ver.rc *.exe lib*dll.def lib*.pc +tags +TAGS # Local excludes in root directory /GNUmakefile
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Wed, 19 Oct 2022 17:17:17 +0900 (JST)
Tatsuo Ishii wrote:
> >> > By the way, in passing, how about adding "tags" and "TAGS" to
> >> > .gitignore file?
> >>
> >> Sounds like a good idea.
> >
> > Ok, the patch is attached.
>
> I have search the mail archive and found this:
>
> https://www.postgresql.org/message-id/flat/CAFcNs%2BrG-DASXzHcecYKvAj%2Brmxi8CpMAgbpGpEK-mjC96F%3DLg%40mail.gmail.com
>
> It seems the consensus was to avoid to put this sort of things into
> .gitignore in the PostgreSQL source tree. Rather, put into personal
> .gitignore or whatever so that developers don't need to care about
> other's preference.
Ok, I understand. Thanks!
By the way, after executing both make_etags and make_ctags, trying tag jump
in my vim causes the following error even though there are correct tags files.
E431: Format error in tags file "backend/access/heap/TAGS"
Removing all TAGS files as below can resolve this error.
$ find . -name TAGS | xargs rm
So, should we have one more option of make_{ce}tags script to clean up
existing tags/TAGS files?
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Wed, 19 Oct 2022 18:11:13 +0900 (JST)
Tatsuo Ishii wrote:
> > By the way, after executing both make_etags and make_ctags, trying tag jump
> > in my vim causes the following error even though there are correct tags
> > files.
> >
> > E431: Format error in tags file "backend/access/heap/TAGS"
> >
> > Removing all TAGS files as below can resolve this error.
> > $ find . -name TAGS | xargs rm
> >
> > So, should we have one more option of make_{ce}tags script to clean up
> > existing tags/TAGS files?
>
> Not sure. Before the commit make_ctags did not do such a thing but we
> never heard any complain like yours. Also I believe vi/vim users never
> invoke make_etags (same thing can be said to emacs users). So why
> should we bother?
Indeed, it was my first use of make_etags (or make_ctags -e) and it was
just for testing the patch. Similarly, someone who runs mistakenly this
command might want this option. However, as you say, there've been no
complain about this, so I don't feel it necessary so much. Maybe, users
of this command would be able to remove tags by their selves easily.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
On Sun, 06 Nov 2022 12:54:17 -0500 Tom Lane wrote: > Yugo NAGATA writes: > >> The attached patch tries to add comments explaining it on the functions. > > > I forward it to the hackers list because the patch is to fix comments. > > What do you think of the attached wording? It looks good to me. That describes the expected behaviour exactly. > I don't think the pipeline angle is of concern to anyone who might be > reading these comments with the aim of understanding what guarantees > they have. Perhaps there should be more about that in the user-facing > docs, though. I agree with that we don't need to mention pipelining in these comments, and that we need more in the documentation. I attached a doc patch to add a mention of commands that do internal commit to the pipelining section. Also, this adds a reference for the pipelining protocol to the libpq doc. Regards, Yugo Nagata -- Yugo NAGATA diff --git a/doc/src/sgml/libpq.sgml b/doc/src/sgml/libpq.sgml index 3c9bd3d673..727a024e60 100644 --- a/doc/src/sgml/libpq.sgml +++ b/doc/src/sgml/libpq.sgml @@ -5058,7 +5058,8 @@ int PQflush(PGconn *conn); While the pipeline API was introduced in PostgreSQL 14, it is a client-side feature which doesn't require special server support and works on any server - that supports the v3 extended query protocol. + that supports the v3 extended query protocol; see . + diff --git a/doc/src/sgml/protocol.sgml b/doc/src/sgml/protocol.sgml index 5fdd429e05..2edd42d7e9 100644 --- a/doc/src/sgml/protocol.sgml +++ b/doc/src/sgml/protocol.sgml @@ -1095,6 +1095,9 @@ SELCT 1/0; that cannot be executed inside a transaction block. If one of these is executed in a pipeline, it will, upon success, force an immediate commit to preserve database consistency. +In addition, execution one of some commands (such as +VACUUM ANALYZE) that start/commit transactions +internally also can cause an immediate commit even if it fails. A Sync immediately following one of these has no effect except to respond with ReadyForQuery.
Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
On Wed, 09 Nov 2022 11:17:29 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > Tom Lane wrote: > >> What do you think of the attached wording? > > > It looks good to me. That describes the expected behaviour exactly. > > Pushed that, then. Thank you. > >> I don't think the pipeline angle is of concern to anyone who might be > >> reading these comments with the aim of understanding what guarantees > >> they have. Perhaps there should be more about that in the user-facing > >> docs, though. > > > I agree with that we don't need to mention pipelining in these comments, > > and that we need more in the documentation. I attached a doc patch to add > > a mention of commands that do internal commit to the pipelining section. > > Also, this adds a reference for the pipelining protocol to the libpq doc. > > Hmm ... I don't really find either of these changes to be improvements. > The fact that, say, multi-table ANALYZE uses multiple transactions > seems to me to be a property of that statement, not of the protocol. Ok. Then, if we want to notice users that commands using internal commits could unexpectedly close a transaction in pipelining, the proper place is libpq section? Regards, Yugo Nagata -- Yugo NAGATA
Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
On Wed, 09 Nov 2022 11:38:05 -0500 Tom Lane wrote: > Peter Eisentraut writes: > > This has broken the following use: > > > parse: create temporary table t1 (a int) on commit drop > > bind > > execute > > parse: analyze t1 > > bind > > execute > > parse: select * from t1 > > bind > > execute > > sync > > > I think the behavior of IsInTransactionBlock() needs to be further > > refined to support this. > > Hmm. Maybe the right way to think about this is "if we have completed an > EXECUTE, and not yet received a following SYNC, then report that we are in > a transaction block"? But I'm not sure if that breaks any other cases. Or, in that case, regarding it as an implicit transaction if multiple commands are executed in a pipeline as proposed in [1] could be another solution, although I have once withdrawn this for not breaking backward compatibility. Attached is the same patch of [1]. [1] https://www.postgresql.org/message-id/20220728105134.d5ce51dd756b3149e9b9c52c%40sraoss.co.jp Regards, Yugo Nagata -- Yugo NAGATA
Re: BUG #17434: CREATE/DROP DATABASE can be executed in the same transaction with other commands
On Thu, 10 Nov 2022 15:33:37 -0500
Tom Lane wrote:
> Yugo NAGATA writes:
> > Tom Lane wrote:
> >> Hmm. Maybe the right way to think about this is "if we have completed an
> >> EXECUTE, and not yet received a following SYNC, then report that we are in
> >> a transaction block"? But I'm not sure if that breaks any other cases.
>
> > Or, in that case, regarding it as an implicit transaction if multiple
> > commands
> > are executed in a pipeline as proposed in [1] could be another solution,
> > although I have once withdrawn this for not breaking backward compatibility.
>
> I didn't like that patch then and I still don't. In particular, it's
> mighty weird to be issuing BeginImplicitTransactionBlock after we've
> already completed one command of the pipeline. If that works without
> obvious warts, it's only accidental.
Ok, I agree with that ugly part of my proposal, so I withdraw it again
if there is another acceptable solution.
> Attached is a draft patch along the lines I speculated about above.
> It breaks backwards compatibility in that PreventInTransactionBlock
> commands will now be rejected if they're a non-first command in a
> pipeline. I think that's okay, and arguably desirable, for HEAD
That patch seems good to me. It fixes the problem reported from
Peter Eisentraut. Also, this seems simple way to define what is
"pipelining" in the code.
> but I'm pretty uncomfortable about back-patching it.
If we want to fix the ANALYZE problem without breaking backward
compatibility for back-patching, maybe we could fix only
IsInTransactionBlock and remain PreventInTransactionBlock as it is.
Obviously, this will break consistency of guarantee between those
functions, but if we are abandoning it eventually, it might be okay.
Anyway, if we change PreventInTransactionBlock to forbid execute
some DDLs in a pipeline, we also need to modify the doc.
> I thought of a variant idea that I think would significantly reduce
> the risk of breaking working applications, which is to restrict things
> only in the case of pipelines with previous data-modifying commands.
> I tried to implement that by having PreventInTransactionBlock test
>
> if (GetTopTransactionIdIfAny() != InvalidTransactionId)
>
> but it blew up, because we have various (mostly partitioning-related)
> DDL commands that run PreventInTransactionBlock only after they've
> acquired an exclusive lock on something, and LogAccessExclusiveLock
> gets an XID. (That was always a horrid POLA-violating kluge that
> would bite us on the rear someday, and now it has. But I can't see
> trying to change that in back branches.)
>
> Something could still be salvaged of the idea, perhaps: we could
> adjust this patch so that the tests are like
>
> if ((MyXactFlags & XACT_FLAGS_PIPELINING) &&
> GetTopTransactionIdIfAny() != InvalidTransactionId)
>
> Maybe that makes it a small enough hazard to be back-patchable.
In this case, DDLs that call PreventInTransactionBlock would be
allowed in a pipeline if any data-modifying commands are yet executed.
This specification is a bit complicated and I'm not sure how many
cases are salvaged by this, but I agree that this will reduce the
hazard of breaking backward-compatibility.
> Another objection that could be raised is the same one I made
> already, that !IsInTransactionBlock() doesn't provide the same
> guarantee as PreventInTransactionBlock. I'm not too happy
> about that either, but given that we know of no other uses of
> IsInTransactionBlock besides ANALYZE, maybe it's okay. I'm
> not sure it's worth trying to avoid it anyway --- we'd just
> end up with a probably-dead backwards compatibility stub.
One way to fix the ANALYZE problem while maintaining the
backward-compatibility for third-party tools using IsInTransactionBlock
might be to rename the function (ex. IsInTransactionBlockWithoutCommit)
and define a new function with the original name.
For example, define the followin for third party tools,
bool IsInTransactionBlock()
{
if (!IsInTransactionBlockWithoutCommit())
{
MyXactFlags |= XACT_FLAGS_NEEDIMMEDIATECOMMIT;
return false;
}
else
return true;
}
and use IsInTransactionBlockWithoutCommit in ANALYZE.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: Generating "Subplan Removed" in EXPLAIN
On Tue, 31 Jan 2023 20:38:21 -0600 Justin Pryzby wrote: > > To: Bruce Momjian > Cc: pgsql-hack...@postgresql.org > Subject: Re: Generating "Subplan Removed" in EXPLAIN > Date: Tue, 31 Jan 2023 20:38:21 -0600 > User-Agent: Mutt/1.9.4 (2018-02-28) > > On Tue, Jan 31, 2023 at 08:59:57PM -0500, Bruce Momjian wrote: > > Does anyone know how to generate this? Thanks. > > The regression tests know: > > $ git grep -c 'Subplans Removed' ./src/test/regress/ > src/test/regr Maybe, you missed to set plan_cache_mode to force_generic_plan. "Subplan Removed" doesn't appear when using a custom plan. postgres=# set enable_indexonlyscan = off; SET postgres=# prepare ab_q1 (int, int, int) as select * from ab where a between $1 and $2 and b <= $3; PREPARE postgres=# explain (analyze, costs off, summary off, timing off) execute ab_q1 (2, 2, 3); QUERY PLAN - Append (actual rows=0 loops=1) -> Seq Scan on ab_a2_b1 ab_1 (actual rows=0 loops=1) Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) -> Seq Scan on ab_a2_b2 ab_2 (actual rows=0 loops=1) Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) -> Seq Scan on ab_a2_b3 ab_3 (actual rows=0 loops=1) Filter: ((a >= 2) AND (a <= 2) AND (b <= 3)) (7 rows) postgres=# show plan_cache_mode ; plan_cache_mode - auto (1 row) postgres=# set plan_cache_mode to force_generic_plan; SET postgres=# explain (analyze, costs off, summary off, timing off) execute ab_q1 (2, 2, 3); QUERY PLAN - Append (actual rows=0 loops=1) Subplans Removed: 6 -> Seq Scan on ab_a2_b1 ab_1 (actual rows=0 loops=1) Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) -> Seq Scan on ab_a2_b2 ab_2 (actual rows=0 loops=1) Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) -> Seq Scan on ab_a2_b3 ab_3 (actual rows=0 loops=1) Filter: ((a >= $1) AND (a <= $2) AND (b <= $3)) (8 rows) Regards, Yugo Nagata -- Yugo NAGATA
Re: Allow an extention to be updated without a script
On Wed, 1 Feb 2023 14:37:27 +0800 Julien Rouhaud wrote: > Hi, > > On Tue, Jan 31, 2023 at 11:17:22AM +0900, Yugo NAGATA wrote: > > > > On Mon, 30 Jan 2023 16:05:52 -0500 > > Tom Lane wrote: > > > > > If you have no update script, why call it a new version? The point > > > of extension versions is to distinguish different states of the > > > extension's SQL objects. We do not consider mods in underlying C code > > > to justify a new version. > > > > Although, as you say, the extension manager doesn't track changes in C code > > functions, a new version could be released with changes in only in C > > functions that implement a new feature or fix some bugs because it looks > > a new version from user's view. Actually, there are several extensions > > that provide empty update scripts in order to allow user to install such > > new versions, for example; > > > > [...] > > - hypopg > > > > (https://github.com/HypoPG/hypopg/blob/REL1_STABLE/hypopg--1.3.1--1.3.2.sql) > > [...] > > Indeed, almost all users don't really understand the difference between the > module / C code and the extension, and that gets worse when > shared_preload_libraries gets in the way. > > I personally choose to ship "empty" extension versions so that people can > upgrade it if they want to have e.g. the OS level version match the SQL level > version. I know some extensions that chose a different approach: keep the > first 2 digits for anything that involves extension changes and have a 3rd > digit for C level bugfix only. But they get very frequent bug reports about > version mismatch any time a C bugfix is released, so I will again personally > keep shipping those useless versions. That being said, I agree with Tom here > and we shouldn't add hacks for that. Thank you for your comment and explanation. That helped me understand extension release approaches. I will withdraw the proposal since just providing empty update scripts can resolve the problem and it wouldn't be worth the confusing. Regards, Yugo Nagata -- Yugo NAGATA
Re: Generating "Subplan Removed" in EXPLAIN
On Wed, 1 Feb 2023 16:52:07 +1300
David Rowley wrote:
> On Wed, 1 Feb 2023 at 15:53, Yugo NAGATA wrote:
> > Maybe, you missed to set plan_cache_mode to force_generic_plan.
> > "Subplan Removed" doesn't appear when using a custom plan.
>
> I wouldn't say that's 100% true. The planner is only able to prune
> using values which are known during planning. Constant folding is
> going to evaluate any immutable functions during planning, but nothing
> more.
>
> Partition pruning might be delayed until execution time if some
> expression that's being compared to the partition key is stable. e.g:
>
> create table rp (t timestamp not null) partition by range(t);
> create table rp2022 partition of rp for values from ('2022-01-01') to
> ('2023-01-01');
> create table rp2023 partition of rp for values from ('2023-01-01') to
> ('2024-01-01');
>
> explain select * from rp where t >= now();
>
> Append (cost=0.00..95.33 rows=1506 width=8)
>Subplans Removed: 1
>-> Seq Scan on rp2023 rp_1 (cost=0.00..43.90 rows=753 width=8)
> Filter: (t >= now())
>
I am sorry for my explanation was not completely correct. Thank you for
your clarification.
Regards,
Yugo Nagata
--
Yugo NAGATA
Re: transition tables and UPDATE
y Toro",
> "bottles": 12, "variety": "Merlot"}
> newrow │ {"year": 2022, "brand": "Sunrise", "winery": "Concha y Toro",
> "bottles": 132, "variety": "Merlot"}
>
> My question is how to obtain the same rows without the LIMIT/OFFSET line
> in the trigger function.
>
>
> Also: how can we "subtract" both JSON blobs so that the 'newrow' only
> contains the members that differ? I would like to have this:
>
> ─[ RECORD 1
> ]
> op │ U
> datetime │ 2023-02-01 01:16:44.704036+01
> oldrow │ {"year": 2021, "brand": "Sunrise", "winery": "Concha y Toro",
> "bottles": 12, "variety": "Chardonnay"}
> newrow │ {"bottles": 108}
> ─[ RECORD 2
> ]
> op │ U
> datetime │ 2023-02-01 01:16:44.704036+01
> oldrow │ {"year": 2022, "brand": "Sunrise", "winery": "Concha y Toro",
> "bottles": 12, "variety": "Merlot"}
> newrow │ {"bottles": 132}
>
> --
> Álvaro HerreraBreisgau, Deutschland — https://www.EnterpriseDB.com/
> "La gente vulgar sólo piensa en pasar el tiempo;
> el que tiene talento, en aprovecharlo"
>
>
--
Yugo NAGATA
Re: RLS makes COPY TO process child tables
On Wed, 01 Feb 2023 12:45:57 +0100 Antonin Houska wrote: > While working on [1] I noticed that if RLS gets enabled, the COPY TO command > includes the contents of child table into the result, although the > documentation says it should not: > > "COPY TO can be used only with plain tables, not views, and does not > copy rows from child tables or child partitions. For example, COPY > table TO copies the same rows as SELECT * FROM ONLY table. The syntax > COPY (SELECT * FROM table) TO ... can be used to dump all of the rows > in an inheritance hierarchy, partitioned table, or view." > > A test case is attached (rls.sql) as well as fix proposal > (copy_rls_no_inh.diff). I think this is a bug because the current behaviour is different from the documentation. When RLS is enabled on a table in `COPY ... TO ...`, the query is converted to `COPY (SELECT * FROM ...) TO ...` to allow the rewriter to add in RLS clauses. This causes to dump the rows of child tables. The patch fixes this by setting "inh" of the table in the converted query to false. This seems reasonable and actually fixes the problem. However, I think we would want a comment on the added line. Also, the attached test should be placed in the regression test. Regards, Yugo Nagata > > [1] https://commitfest.postgresql.org/41/3641/ > > -- > Antonin Houska > Web: https://www.cybertec-postgresql.com > -- Yugo NAGATA
Re: RLS makes COPY TO process child tables
On Wed, 01 Feb 2023 11:47:23 -0500 Tom Lane wrote: > Yugo NAGATA writes: > > Antonin Houska wrote: > >> While working on [1] I noticed that if RLS gets enabled, the COPY TO > >> command > >> includes the contents of child table into the result, although the > >> documentation says it should not: > > > I think this is a bug because the current behaviour is different from > > the documentation. > > I agree, it shouldn't do that. > > > When RLS is enabled on a table in `COPY ... TO ...`, the query is converted > > to `COPY (SELECT * FROM ...) TO ...` to allow the rewriter to add in RLS > > clauses. This causes to dump the rows of child tables. > > Do we actually say that in so many words, either in the code or docs? > If so, it ought to read `COPY (SELECT * FROM ONLY ...) TO ...` > instead. (If we say that in the docs, then arguably the code *does* > conform to the docs. But I don't see it in the COPY ref page at least.) The documentation do not say that, but the current code actually do that. Also, there is the following comment in BeginCopyTo(). * With row-level security and a user using "COPY relation TO", we * have to convert the "COPY relation TO" to a query-based COPY (eg: * "COPY (SELECT * FROM relation) TO"), to allow the rewriter to add * in any RLS clauses. Maybe, it is be better to change the description in the comment to "COPY (SELECT * FROM ONLY relation) TO" when fixing the bug. Regards, Yugo Nagata -- Yugo NAGATA
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Tue, 07 Feb 2023 17:19:37 +0900 (JST)
Tatsuo Ishii wrote:
> >> Since this commit, make_etags has started failing to generate
> >> tags files with the following error messages, on my MacOS.
> >>
> >> $ src/tools/make_etags
> >> /Applications/Xcode.app/Contents/Developer/Toolchains/XcodeDefault.xctoolchain/usr/bin/ctags:
> >> illegal option -- e
> >> usage: ctags [-BFadtuwvx] [-f tagsfile] file ...
> >> sort: No such file or directory
> >>
> >>
> >> In my MacOS, non-Exuberant ctags is installed and doesn't support
> >> -e option. But the commit changed make_etags so that it always
> >> calls ctags with -e option via make_ctags. This seems the cause of
> >> the above failure.
> >>
> >> IS_EXUBERANT=""
> >> ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
> >>
> >> make_ctags has the above code and seems to support non-Exuberant
> >> ctags.
> >> If so, we should revert the changes of make_etags by the commit and
> >> make make_etags work with that ctags? Or, we should support
> >> only Exuberant-type ctags (btw, I'm ok with this) and get rid of
> >> something like the above code?
> >
> > Thanks for the report. I will look into this.
>
> Previous make_etags relied on etags command:
>
> #!/bin/sh
>
> # src/tools/make_etags
>
> command -v etags >/dev/null || \
> { echo "'etags' program not found" 1>&2; exit 1; }
> :
> :
>
> My Mac (M1 Mac running macOS 12.6) does not have etags. Thus before
> the commit make_etags on Mac failed anyway. Do we want make_etags to
> restore the previous behavior? i.e. 'etags' program not found
>
> >> If so, we should revert the changes of make_etags by the commit and
> >> make make_etags work with that ctags?
>
> I think ctags on Mac cannot produce tags file for emacs.
Does is make sense to change make_etags as the attached patch does?
This allows make_etags to use etags if Exuberant-type ctags is not
available. This allows users to use make_etags if hey has either
Exuberant-type ctags or etags.
Regards,
Yugo Nagata
>
> Best reagards,
> --
> Tatsuo Ishii
> SRA OSS LLC
> English: http://www.sraoss.co.jp/index_en/
> Japanese:http://www.sraoss.co.jp
--
Yugo NAGATA
diff --git a/src/tools/make_etags b/src/tools/make_etags
index afc57e3e89..d0cfdb23d8 100755
--- a/src/tools/make_etags
+++ b/src/tools/make_etags
@@ -1,6 +1,25 @@
#!/bin/sh
# src/tools/make_etags
-cdir=`dirname $0`
-dir=`(cd $cdir && pwd)`
-exec $dir/make_ctags -e $*
+IS_EXUBERANT=""
+command -v ctags >/dev/null && \
+ctags --version 2>&1 | grep Exuberant && IS_EXUBERANT="Y"
+
+if [ "$IS_EXUBERANT" ]; then
+cdir=`dirname $0`
+dir=`(cd $cdir && pwd)`
+exec $dir/make_ctags -e $*
+else
+command -v etags >/dev/null || \
+{ echo "'etags' program not found" 1>&2; exit 1; }
+
+rm -f ./TAGS
+
+find `pwd`/ -type f -name '*.[chyl]' -print |
+xargs etags --append -o TAGS
+
+find . \( -name CVS -prune \) -o \( -name .git -prune \) -o -type d -print |
+while read DIR
+do[ "$DIR" != "." ] && ln -f -s `pwd`/TAGS "$DIR"
+done
+fi
Re: make_ctags: use -I option to ignore pg_node_attr macro
On Tue, 07 Feb 2023 21:29:04 +0900 (JST) Tatsuo Ishii wrote: > > Does is make sense to change make_etags as the attached patch does? > > This allows make_etags to use etags if Exuberant-type ctags is not > > available. This allows users to use make_etags if hey has either > > Exuberant-type ctags or etags. > > The patch drops support for "-n" option :-< > > Attached is the patch by fixing make_ctags (make_etags is not > touched). > > If Exuberant-type ctags is available, use it (not changed). > If Exuberant-type ctags is not available, try old ctags (not changed). > If the old ctags does not support "-e" option, try etags (new). I am not sure if this is good way to check if ctags supports "-e" or not. + thenctags --version 2>&1 | grep -- -e >/dev/null Perhaps, "--help" might be intended rather than "--version" to check supported options? Even so, ctags would have other option whose name contains "-e" than Emacs support, so this check could end in a wrong result. Therefore, it seems to me that it is better to check immediately if etags is available in case that we don't have Exuberant-type ctags. Regards, Yugo Nagata > If etags is not available, give up (new). > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA
doc: a small improvement about pg_am description
Hi, When reading the documentation about operator class, I found the following description: The pg_am table contains one row for every index access method. Support for access to regular tables is built into PostgreSQL, but all index access methods are described in pg_am. It seems to me that this description says pg_am contains only index access methods but not table methods. I wonder it is missed to fix this when tableam was supported and other documentation was changed in b73c3a11963c8bb783993cfffabb09f558f86e37. Attached is a patch to remove the sentence that starts with "Support for access to regular tables is ". Ragards, Yugo Nagata -- Yugo NAGATA diff --git a/doc/src/sgml/xindex.sgml b/doc/src/sgml/xindex.sgml index c753d8005a..ff73233818 100644 --- a/doc/src/sgml/xindex.sgml +++ b/doc/src/sgml/xindex.sgml @@ -30,11 +30,8 @@ The pg_am table contains one row for every - index method (internally known as access method). Support for - regular access to tables is built into - PostgreSQL, but all index methods are - described in pg_am. It is possible to add a - new index access method by writing the necessary code and + index and table method (internally known as access method). It is possible + to add a new index access method by writing the necessary code and then creating an entry in pg_am — but that is beyond the scope of this chapter (see ).
Re: pgbench: allow to exit immediately when any client is aborted
On Sat, 19 Aug 2023 19:51:56 +0900 (JST) Tatsuo Ishii wrote: > > I have tested the v4 patch with default_transaction_isolation = > > 'repeatable read'. > > > > pgbench --exit-on-abort -N -p 11002 -c 10 -T 30 test > > pgbench (17devel, server 15.3) > > starting vacuum...end. > > transaction type: > > scaling factor: 1 > > query mode: simple > > number of clients: 10 > > number of threads: 1 > > maximum number of tries: 1 > > duration: 30 s > > number of transactions actually processed: 64854 > > number of failed transactions: 4 (0.006%) > > latency average = 4.628 ms (including failures) > > initial connection time = 49.526 ms > > tps = 2160.827556 (without initial connection time) > > > > Depite the 4 failed transactions (could not serialize access due to > > concurrent update) pgbench ran normally, rather than aborting the > > test. Is this an expected behavior? Yes. --exit-on-abort changes a behaviour when a client is **aborted** due to an error, and serialization errors do not cause abort, so it is not affected. > I start to think this behavior is ok and consistent with previous > behavior of pgbench because serialization (and dealock) errors have > been treated specially from other types of errors, such as accessing > non existing tables. However, I suggest to add more sentences to the > explanation of this option so that users are not confused by this. > > + > + --exit-on-abort > + > + > +Exit immediately when any client is aborted due to some error. > Without > +this option, even when a client is aborted, other clients could > continue > +their run as specified by -t or -T > option, > +and pgbench will print an incomplete > results > +in this case. > + > + > + > + > > What about inserting "Note that serialization failures or deadlock > failures will not abort client. See linkend="failures-and-retries"/> for more information." into the end > of this paragraph? --exit-on-abort is related to "abort" of a client instead of error or failure itself, so rather I wonder a bit that mentioning serialization/deadlock failures might be confusing. However, if users may think of such failures from "abort", it could be beneficial to add the sentences with some modification as below. "Note that serialization failures or deadlock failures does not abort the client, so they are not affected by this option. See for more information." > BTW, I think: > Exit immediately when any client is aborted due to some error. Without > > should be: > Exit immediately when any client is aborted due to some errors. > Without > > (error vs. erros) Well, I chose "some" to mean "unknown or unspecified", not "an unspecified amount or number of", so singular form "error" is used. Instead, should we use "due to a error"? > Also: > + --exit-on-abort is specified . Otherwise in the > worst > > There is an extra space between "specified" and ".". Fixed. Also, I fixed the place of the description in the documentation to alphabetical order Attached is the updated patch. Regards, Yugo Nagata > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 6c5c8afa6d..4175cf4ccd 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -768,6 +768,24 @@ pgbench options d + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + +Note that serialization failures or deadlock failures does not abort the +client, so they are not affected by this option. +See for more information. + + + + --failures-detailed @@ -985,7 +1003,8 @@ pgbench options d benchmark such as initial connection failures also exit with status 1. Errors during the run such as database errors or problems in the script will result in exit status 2. In the latter case, - pgbench will print partial results. + pgbench will print partial results if + --exit-on-abort option is not specified. @@ -2801,14 +2820,17 @@
Re: pgbench: allow to exit immediately when any client is aborted
On Thu, 24 Aug 2023 09:15:51 +0900 (JST) Tatsuo Ishii wrote: > >> I start to think this behavior is ok and consistent with previous > >> behavior of pgbench because serialization (and dealock) errors have > >> been treated specially from other types of errors, such as accessing > >> non existing tables. However, I suggest to add more sentences to the > >> explanation of this option so that users are not confused by this. > >> > >> + > >> + --exit-on-abort > >> + > >> + > >> +Exit immediately when any client is aborted due to some error. > >> Without > >> +this option, even when a client is aborted, other clients could > >> continue > >> +their run as specified by -t or > >> -T option, > >> +and pgbench will print an incomplete > >> results > >> +in this case. > >> + > >> + > >> + > >> + > >> > >> What about inserting "Note that serialization failures or deadlock > >> failures will not abort client. See >> linkend="failures-and-retries"/> for more information." into the end > >> of this paragraph? > > > > --exit-on-abort is related to "abort" of a client instead of error or > > failure itself, so rather I wonder a bit that mentioning > > serialization/deadlock > > failures might be confusing. However, if users may think of such failures > > from > > "abort", it could be beneficial to add the sentences with some modification > > as > > below. > > I myself confused by this and believe that adding extra paragraph is > beneficial to users. Ok. > > "Note that serialization failures or deadlock failures does not abort the > > client, so they are not affected by this option. > > See for more information." > > "does not" --> "do not". Oops. I attached the updated patch. > >> BTW, I think: > >> Exit immediately when any client is aborted due to some error. > >> Without > >> > >> should be: > >> Exit immediately when any client is aborted due to some errors. > >> Without > >> > >> (error vs. erros) > > > > Well, I chose "some" to mean "unknown or unspecified", not "an unspecified > > amount > > or number of", so singular form "error" is used. > > Ok. > > > Instead, should we use "due to a error"? > > I don't think so. > > >> Also: > >> + --exit-on-abort is specified . Otherwise in the > >> worst > >> > >> There is an extra space between "specified" and ".". > > > > Fixed. > > > > Also, I fixed the place of the description in the documentation > > to alphabetical order > > > > Attached is the updated patch. > > Looks good to me. If there's no objection, I will commit this next week. > > Best reagards, > -- > Tatsuo Ishii > SRA OSS LLC > English: http://www.sraoss.co.jp/index_en/ > Japanese:http://www.sraoss.co.jp -- Yugo NAGATA diff --git a/doc/src/sgml/ref/pgbench.sgml b/doc/src/sgml/ref/pgbench.sgml index 6c5c8afa6d..95cc9027c3 100644 --- a/doc/src/sgml/ref/pgbench.sgml +++ b/doc/src/sgml/ref/pgbench.sgml @@ -768,6 +768,24 @@ pgbench options d + + --exit-on-abort + + +Exit immediately when any client is aborted due to some error. Without +this option, even when a client is aborted, other clients could continue +their run as specified by -t or -T option, +and pgbench will print an incomplete results +in this case. + + +Note that serialization failures or deadlock failures do not abort the +client, so they are not affected by this option. +See for more information. + + + + --failures-detailed @@ -985,7 +1003,8 @@ pgbench options d benchmark such as initial connection failures also exit with status 1. Errors during the run such as database errors or problems in the script will result in exit status 2. In the latter case, - pgbench will print partial results. + pgbench will print partial results if + --exit-on-abort option is not specified. @@ -2801,14 +2820,17 @@ statement latencies in milliseconds, failures and retries: start a connection to the database server / the socket for connecting the client to the database server has become in
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 00:40:45 +0800 jian he wrote: > On Wed, Jun 28, 2023 at 4:06 PM Yugo NAGATA wrote: > > > > On Wed, 28 Jun 2023 00:01:02 +0800 > > jian he wrote: > > > > > On Thu, Jun 1, 2023 at 2:47 AM Yugo NAGATA wrote: > > > > > > > > On Thu, 1 Jun 2023 23:59:09 +0900 > > > > Yugo NAGATA wrote: > > > > > > > > > Hello hackers, > > > > > > > > > > Here's a rebased version of the patch-set adding Incremental View > > > > > Maintenance support for PostgreSQL. That was discussed in [1]. > > > > > > > > > [1] > > > > > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp > > > > > > > > --- > > > > * Overview > > > > > > > > Incremental View Maintenance (IVM) is a way to make materialized views > > > > up-to-date by computing only incremental changes and applying them on > > > > views. IVM is more efficient than REFRESH MATERIALIZED VIEW when > > > > only small parts of the view are changed. > > > > > > > > ** Feature > > > > > > > > The attached patchset provides a feature that allows materialized views > > > > to be updated automatically and incrementally just after a underlying > > > > table is modified. > > > > > > > > You can create an incementally maintainable materialized view (IMMV) > > > > by using CREATE INCREMENTAL MATERIALIZED VIEW command. > > > > > > > > The followings are supported in view definition queries: > > > > - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) > > > > - some built-in aggregate functions (count, sum, avg, min, max) > > > > - GROUP BY clause > > > > - DISTINCT clause > > > > > > > > Views can contain multiple tuples with the same content (duplicate > > > > tuples). > > > > > > > > ** Restriction > > > > > > > > The following are not supported in a view definition: > > > > - Outer joins > > > > - Aggregates otehr than above, window functions, HAVING > > > > - Sub-queries, CTEs > > > > - Set operations (UNION, INTERSECT, EXCEPT) > > > > - DISTINCT ON, ORDER BY, LIMIT, OFFSET > > > > > > > > Also, a view definition query cannot contain other views, materialized > > > > views, > > > > foreign tables, partitioned tables, partitions, VALUES, non-immutable > > > > functions, > > > > system columns, or expressions that contains aggregates. > > > > > > > > --- > > > > * Design > > > > > > > > An IMMV is maintained using statement-level AFTER triggers. > > > > When an IMMV is created, triggers are automatically created on all base > > > > tables contained in the view definition query. > > > > > > > > When a table is modified, changes that occurred in the table are > > > > extracted > > > > as transition tables in the AFTER triggers. Then, changes that will > > > > occur in > > > > the view are calculated by a rewritten view dequery in which the > > > > modified table > > > > is replaced with the transition table. > > > > > > > > For example, if the view is defined as "SELECT * FROM R, S", and tuples > > > > inserted > > > > into R are stored in a transiton table dR, the tuples that will be > > > > inserted into > > > > the view are calculated as the result of "SELECT * FROM dR, S". > > > > > > > > ** Multiple Tables Modification > > > > > > > > Multiple tables can be modified in a statement when using triggers, > > > > foreign key > > > > constraint, or modifying CTEs. When multiple tables are modified, we > > > > need > > > > the state of tables before the modification. > > > > > > > > For example, when some tuples, dR and dS, are inserted into R and S > > > > respectively, > > > > the tuples that will be inserted into the view are calculated by the > > > > following > > > > two queries: > > > > > > > > &quo
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 18:20:32 +0800 jian he wrote: > On Thu, Jun 29, 2023 at 12:40 AM jian he wrote: > > > > On Wed, Jun 28, 2023 at 4:06 PM Yugo NAGATA wrote: > > > > > > On Wed, 28 Jun 2023 00:01:02 +0800 > > > jian he wrote: > > > > > > > On Thu, Jun 1, 2023 at 2:47 AM Yugo NAGATA wrote: > > > > > > > > > > On Thu, 1 Jun 2023 23:59:09 +0900 > > > > > Yugo NAGATA wrote: > > > > > > > > > > > Hello hackers, > > > > > > > > > > > > Here's a rebased version of the patch-set adding Incremental View > > > > > > Maintenance support for PostgreSQL. That was discussed in [1]. > > > > > > > > > > > [1] > > > > > > https://www.postgresql.org/message-id/flat/20181227215726.4d166b4874f8983a641123f5%40sraoss.co.jp > > > > > > > > > > --- > > > > > * Overview > > > > > > > > > > Incremental View Maintenance (IVM) is a way to make materialized views > > > > > up-to-date by computing only incremental changes and applying them on > > > > > views. IVM is more efficient than REFRESH MATERIALIZED VIEW when > > > > > only small parts of the view are changed. > > > > > > > > > > ** Feature > > > > > > > > > > The attached patchset provides a feature that allows materialized > > > > > views > > > > > to be updated automatically and incrementally just after a underlying > > > > > table is modified. > > > > > > > > > > You can create an incementally maintainable materialized view (IMMV) > > > > > by using CREATE INCREMENTAL MATERIALIZED VIEW command. > > > > > > > > > > The followings are supported in view definition queries: > > > > > - SELECT ... FROM ... WHERE ..., joins (inner joins, self-joins) > > > > > - some built-in aggregate functions (count, sum, avg, min, max) > > > > > - GROUP BY clause > > > > > - DISTINCT clause > > > > > > > > > > Views can contain multiple tuples with the same content (duplicate > > > > > tuples). > > > > > > > > > > ** Restriction > > > > > > > > > > The following are not supported in a view definition: > > > > > - Outer joins > > > > > - Aggregates otehr than above, window functions, HAVING > > > > > - Sub-queries, CTEs > > > > > - Set operations (UNION, INTERSECT, EXCEPT) > > > > > - DISTINCT ON, ORDER BY, LIMIT, OFFSET > > > > > > > > > > Also, a view definition query cannot contain other views, > > > > > materialized views, > > > > > foreign tables, partitioned tables, partitions, VALUES, non-immutable > > > > > functions, > > > > > system columns, or expressions that contains aggregates. > > > > > > > > > > --- > > > > > * Design > > > > > > > > > > An IMMV is maintained using statement-level AFTER triggers. > > > > > When an IMMV is created, triggers are automatically created on all > > > > > base > > > > > tables contained in the view definition query. > > > > > > > > > > When a table is modified, changes that occurred in the table are > > > > > extracted > > > > > as transition tables in the AFTER triggers. Then, changes that will > > > > > occur in > > > > > the view are calculated by a rewritten view dequery in which the > > > > > modified table > > > > > is replaced with the transition table. > > > > > > > > > > For example, if the view is defined as "SELECT * FROM R, S", and > > > > > tuples inserted > > > > > into R are stored in a transiton table dR, the tuples that will be > > > > > inserted into > > > > > the view are calculated as the result of "SELECT * FROM dR, S". > > > > > > > > > > ** Multiple Tables Modification > > > > > > > > > > Multiple tables can be modified in a statement when using triggers, > > > > >
Re: Incremental View Maintenance, take 2
On Thu, 29 Jun 2023 18:51:06 +0800
jian he wrote:
> I cannot build the doc.
> git clean -fdx
> git am ~/Desktop/tmp/*.patch
>
> Applying: Add a syntax to create Incrementally Maintainable Materialized Views
> Applying: Add relisivm column to pg_class system catalog
> Applying: Allow to prolong life span of transition tables until transaction
> end
> Applying: Add Incremental View Maintenance support to pg_dump
> Applying: Add Incremental View Maintenance support to psql
> Applying: Add Incremental View Maintenance support
> Applying: Add DISTINCT support for IVM
> Applying: Add aggregates support in IVM
> Applying: Add support for min/max aggregates for IVM
> Applying: Add regression tests for Incremental View Maintenance
> Applying: Add documentations about Incremental View Maintenance
> .git/rebase-apply/patch:79: trailing whitespace.
> clause.
> warning: 1 line adds whitespace errors.
>
> Because of this, the {ninja docs} command failed. ERROR message:
>
> [6/6] Generating doc/src/sgml/html with a custom command
> FAILED: doc/src/sgml/html
> /usr/bin/python3
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml/xmltools_dep_wrapper
> --targetname doc/src/sgml/html --depfile doc/src/sgml/html.d --tool
> /usr/bin/xsltproc -- -o doc/src/sgml/ --nonet --stringparam pg.version
> 16beta2 --path doc/src/sgml --path
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml
> ../../Desktop/pg_sources/main/postgres/doc/src/sgml/stylesheet.xsl
> doc/src/sgml/postgres-full.xml
> ERROR: id attribute missing on element under /book[@id =
> 'postgres']/part[@id = 'server-programming']/chapter[@id =
> 'rules']/sect1[@id = 'rules-ivm']
> error: file doc/src/sgml/postgres-full.xml
> xsltRunStylesheet : run failed
> ninja: build stopped: subcommand failed.
Thank your for pointing out this.
I'll add ids for all sections to suppress the errors.
Regards,
Yugo Nagata
--
Yugo NAGATA
