Re: Default to TIMESTAMP WITH TIME ZONE?
On 13.08.21 19:07, Tom Lane wrote: "David G. Johnston" writes: On Fri, Aug 13, 2021 at 9:28 AM Simon Riggs wrote: The only hope is to eventually change the default, so probably the best thing is to apply pressure via the SQL Std process. Then there is no hope because this makes the situation worse. Agreed; the points I made upthread are just as valid if the change is made in the standard. But I'd be astonished if the SQL committee would consider such a change anyway. AFAIU, our timestamp with time zone type doesn't really do what the SQL standard specifies anyway, as it doesn't actually record the time zone, but it's more of a "timestamp with time zone aware formatting". For SQL, it might make sense to add a (third) time stamp type that behaves more like that.
Re: Added schema level support for publication.
On 13.08.21 04:59, Amit Kapila wrote: Even if we drop all tables added to the publication from it, 'pubkind' doesn't go back to 'empty'. Is that intentional behavior? If we do that, we can save the lookup of pg_publication_rel and pg_publication_schema in some cases, and we can switch the publication that was created as FOR SCHEMA to FOR TABLE and vice versa. Do we really want to allow users to change a publication that is FOR SCHEMA to FOR TABLE? I see that we don't allow to do that FOR TABLES. postgres=# Alter Publication pub add table tbl1; ERROR: publication "pub" is defined as FOR ALL TABLES DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES publications. I think the strict separation between publication-for-tables vs. publication-for-schemas is a mistake. Why can't I have a publication that publishes tables t1, t2, t3, *and* schemas s1, s2, s3. Also note that we have a pending patch to add sequences support to logical replication. So eventually, a publication will be able to contain a bunch of different objects of different kinds.
Re: Default to TIMESTAMP WITH TIME ZONE?
On Sat, 14 Aug 2021 at 09:03, Peter Eisentraut wrote: > > On 13.08.21 19:07, Tom Lane wrote: > > "David G. Johnston" writes: > >> On Fri, Aug 13, 2021 at 9:28 AM Simon Riggs > >> wrote: > >>> The only hope is to eventually change the default, so probably > >>> the best thing is to apply pressure via the SQL Std process. > > > >> Then there is no hope because this makes the situation worse. > > > > Agreed; the points I made upthread are just as valid if the change > > is made in the standard. But I'd be astonished if the SQL committee > > would consider such a change anyway. > > AFAIU, our timestamp with time zone type doesn't really do what the SQL > standard specifies anyway, as it doesn't actually record the time zone, > but it's more of a "timestamp with time zone aware formatting". For > SQL, it might make sense to add a (third) time stamp type that behaves > more like that. Hmm, a new datatype would make sense, but I would go in the direction of usability, since that's where this thread started. It would also make sense to have a type called timestampUTC, where 1. all inputs that specify a timezone are converted to UTC 2. all inputs that do not specify a timezone are assumed to be UTC, ignoring the setting of time zone 3. output is not affected by the session time zone, so wherever you look at it from, you see UTC values This allows the UTC Everywhere design pattern and also ensures that all functions are immutable, so will not cause optimization problems. This is useful because using TIMESTAMP WITHOUT TIME ZONE for UTC Everywhere only works if nobody ever sets their time zone, which every user can do. -- Simon Riggshttp://www.EnterpriseDB.com/
Re: Added schema level support for publication.
On Sat, Aug 14, 2021 at 3:02 PM Peter Eisentraut wrote: > > On 13.08.21 04:59, Amit Kapila wrote: > >> Even if we drop all tables added to the publication from it, 'pubkind' > >> doesn't go back to 'empty'. Is that intentional behavior? If we do > >> that, we can save the lookup of pg_publication_rel and > >> pg_publication_schema in some cases, and we can switch the publication > >> that was created as FOR SCHEMA to FOR TABLE and vice versa. > >> > > Do we really want to allow users to change a publication that is FOR > > SCHEMA to FOR TABLE? I see that we don't allow to do that FOR TABLES. > > postgres=# Alter Publication pub add table tbl1; > > ERROR: publication "pub" is defined as FOR ALL TABLES > > DETAIL: Tables cannot be added to or dropped from FOR ALL TABLES > > publications. > > I think the strict separation between publication-for-tables vs. > publication-for-schemas is a mistake. Why can't I have a publication > that publishes tables t1, t2, t3, *and* schemas s1, s2, s3. Also note > that we have a pending patch to add sequences support to logical > replication. So eventually, a publication will be able to contain a > bunch of different objects of different kinds. > Valid point. -- With Regards, Amit Kapila.
Re: Added schema level support for publication.
On Mon, Aug 9, 2021 at 9:50 PM Mark Dilger wrote: > > > > > On Aug 6, 2021, at 1:32 AM, vignesh C wrote: > > > > the attached v19 patch > > With v19 applied, a schema owner can publish the contents of a table regardless of ownership or permissions on that table: > > +CREATE ROLE user1; > +GRANT CREATE ON DATABASE regression TO user1; > +CREATE ROLE user2; > +GRANT CREATE ON DATABASE regression TO user2; > +SET SESSION AUTHORIZATION user1; > +CREATE SCHEMA user1schema; > +GRANT CREATE, USAGE ON SCHEMA user1schema TO user2; > +RESET SESSION AUTHORIZATION; > +SET SESSION AUTHORIZATION user2; > +CREATE TABLE user1schema.user2private (junk text); > +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM PUBLIC; > +REVOKE ALL PRIVILEGES ON user1schema.user2private FROM user1; > +CREATE TABLE user1schema.user2public (junk text); > +GRANT SELECT ON user1schema.user2public TO PUBLIC; > +RESET SESSION AUTHORIZATION; > +SET SESSION AUTHORIZATION user1; > +SELECT * FROM user1schema.user2private; > +ERROR: permission denied for table user2private > +SELECT * FROM user1schema.user2public; > + junk > +-- > +(0 rows) > + > +CREATE PUBLICATION user1pub; > +WARNING: wal_level is insufficient to publish logical changes > +HINT: Set wal_level to logical before creating subscriptions. > +ALTER PUBLICATION user1pub > + ADD TABLE user1schema.user2public; > +ERROR: must be owner of table user2public > +ALTER PUBLICATION user1pub > + ADD TABLE user1schema.user2private, user1schema.user2public; > +ERROR: must be owner of table user2private > +SELECT * FROM pg_catalog.pg_publication_tables > + WHERE pubname = 'user1pub'; > + pubname | schemaname | tablename > +-++--- > +(0 rows) > + > +ALTER PUBLICATION user1pub ADD SCHEMA user1schema; > +SELECT * FROM pg_catalog.pg_publication_tables > + WHERE pubname = 'user1pub'; > + pubname | schemaname | tablename > +--+-+-- > + user1pub | user1schema | user2private > + user1pub | user1schema | user2public > +(2 rows) > > It is a bit counterintuitive that schema owners do not have administrative privileges over tables within their schemas, but that's how it is. The design of this patch seems to assume otherwise. Perhaps ALTER PUBLICATION ... ADD SCHEMA should be restricted to superusers, just as FOR ALL TABLES? Thanks for the comment, this is handled in the v20 patch attached at [1]. > Alternatively, you could add ownership checks per table to mirror the behavior of ALTER PUBLICATION ... ADD TABLE, but that would foreclose the option of automatically updating the list of tables in the publication as new tables are added to the schema, since those new tables would not necessarily belong to the schema owner, and having a error thrown during CREATE TABLE would be quite unfriendly. I think until this is hammered out, it is safer to require superuser privileges and then we can revisit this issue and loosen the requirement in a subsequent commit. I agree with Amit on this point for handling this as a separate patch, I have not made any change for this, kept the behavior as is. [1] - https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com Regards, Vignesh
Re: Added schema level support for publication.
On Tue, Aug 10, 2021 at 1:40 PM Greg Nancarrow wrote: > > On Fri, Aug 6, 2021 at 6:32 PM vignesh C wrote: > > > > Thanks for the comments, the attached v19 patch has the fixes for the comments. > > > > Some more review comments, this time for the v19 patch: > > > (1) In patch v19-0002, there's still a couple of instances where it > says "publication type" instead of "publication kind". Modified > (2) src/backend/catalog/pg_publication.c > > "This should only be used for normal publications." > > What exactly is meant by that - what type is considered normal? Maybe > that comment should be more specific. Modified > (3) src/backend/catalog/pg_publication.c > GetSchemaPublications > > Function header says "Gets list of publication oids for publications > marked as FOR SCHEMA." > > Shouldn't it say something like: "Gets the list of FOR SCHEMA > publication oids associated with a specified schema oid." or something > like that? > (since the function accepts a schemaid parameter) Modfified > (4) src/backend/commands/publicationcmds.c > > In AlterPublicationSchemas(), I notice that the two error cases > "cannot be added to or dropped ..." don't check stmt->action for > DEFELEM_ADD/DEFELEM_DROP. > Is that OK? (i.e. should these cases error out if stmt->action is not > DEFELEM_ADD/DEFELEM_DROP?) > Also, I assume that the else part (not DEFELEM_ADD/DEFELEM_DROP) is > the "Set" case? Maybe a comment should be added to the top of the else > part. The error message should also include set, I have modified the error message accordingly. > (5) src/backend/commands/publicationcmds.c > Typo (same in 2 places): "automaically" -> "automatically" > > + * will be released automaically at the end of create publication > > See functions: > (i) CreatePublication > (ii) AlterPublicationSchemas Modified. > (6) src/backend/commands/publicationcmds.c > LockSchemaList > > Function header says "are locked in ShareUpdateExclusiveLock mode" but > then code calls LockDatabaseObject using "AccessShareLock". Modified. Thanks for the comments, these issues are fixed in the v20 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com Regards, Vignesh
Re: Added schema level support for publication.
On Thu, Aug 12, 2021 at 5:54 PM Masahiko Sawada
wrote:
>
> On Fri, Aug 6, 2021 at 5:33 PM vignesh C wrote:
> >
> > Thanks for the comments, the attached v19 patch has the fixes for the
comments.
>
> Thank you for updating the patch!
>
> Here are some comments on v19 patch:
>
> +case OCLASS_PUBLICATION_SCHEMA:
> +RemovePublicationSchemaById(object->objectId);
> +break;
>
> +void
> +RemovePublicationSchemaById(Oid psoid)
> +{
> +Relation rel;
> +HeapTuple tup;
> +
> +rel = table_open(PublicationSchemaRelationId, RowExclusiveLock);
> +
> +tup = SearchSysCache1(PUBLICATIONSCHEMA,
ObjectIdGetDatum(psoid));
> +
> +if (!HeapTupleIsValid(tup))
> +elog(ERROR, "cache lookup failed for publication schema
%u",
> + psoid);
> +
> +CatalogTupleDelete(rel, &tup->t_self);
> +
> +ReleaseSysCache(tup);
> +
> +table_close(rel, RowExclusiveLock);
> +}
>
> Since RemovePublicationSchemaById() does simple catalog tuple
> deletion, it seems to me that we can DropObjectById() to delete the
> row of pg_publication_schema.
Relation cache invalidations were missing in the function, I have added and
retained the function with invalidation changes.
> ---
> {
> -ScanKeyInit(&key[0],
> +ScanKeyData skey[1];
> +
> +ScanKeyInit(&skey[0],
> Anum_pg_class_relkind,
> BTEqualStrategyNumber, F_CHAREQ,
>
> CharGetDatum(RELKIND_PARTITIONED_TABLE));
>
> -scan = table_beginscan_catalog(classRel, 1, key);
> +scan = table_beginscan_catalog(classRel, 1, skey);
>
> Since we specify 1 as the number of keys in table_beginscan_catalog(),
> can we reuse 'key' instead of using 'skey'?
Modified.
> ---
> Even if we drop all tables added to the publication from it, 'pubkind'
> doesn't go back to 'empty'. Is that intentional behavior? If we do
> that, we can save the lookup of pg_publication_rel and
> pg_publication_schema in some cases, and we can switch the publication
> that was created as FOR SCHEMA to FOR TABLE and vice versa.
I felt this can be handled as a separate patch as the same scenario applies
for all tables publication too. Thoughts?
> ---
> +static void
> +UpdatePublicationKindTupleValue(Relation rel, HeapTuple tup, int col,
> +char
pubkind)
>
> Since all callers of this function specify Anum_pg_publication_pubkind
> to 'col', it seems 'col' is not necessary.
Modified
> ---
> +static void
> +AlterPublicationSchemas(AlterPublicationStmt *stmt, Relation rel,
> +HeapTuple tup,
> Form_pg_publication pubform)
>
> I think we don't need to pass 'pubform' to this function since we can
> get it by GETSTRUCT(tup).
Modified.
> ---
> +OidschemaId =
get_rel_namespace(relid);
> List *pubids = GetRelationPublications(relid);
> +List *schemaPubids =
GetSchemaPublications(schemaId);
>
> Can we defer to get the list of schema publications (i.g.,
> 'schemaPubids') until we find the PUBKIND_SCHEMA publication? Perhaps
> the same is true for building 'pubids'.
I felt that we can get the publication information and use it whenever
required instead of querying in the loop. Thoughts?
> ---
> + List of publications
> +Name| Owner | All tables | Inserts
> | Updates | Deletes | Truncates | Via root | PubKind
>
++--++-+-+-+---+--+-
> + testpib_ins_trunct | regress_publication_user | f | t
> | f | f | f | f| e
> + testpub_default| regress_publication_user | f | f
> | t | f | f | f| e
>
> I think it's more readable if \dRp shows 'all tables', 'table',
> 'schema', and 'empty' in PubKind instead of the single character.
Modified
> I think 'Pub kind' is more consistent with other column names.
Modified
Thanks for the comments, these issues are fixed in the v20 patch attached
at [1].
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com
Regards,
Vignesh
Re: Added schema level support for publication.
On Mon, Aug 9, 2021 at 10:23 AM Amit Kapila wrote:
>
> On Sun, Aug 8, 2021 at 2:52 PM vignesh C wrote:
> >
> > On Fri, Aug 6, 2021 at 4:39 PM Amit Kapila
wrote:
> > >
> > > On Fri, Aug 6, 2021 at 2:02 PM vignesh C wrote:
> > > >
> > > > On Wed, Aug 4, 2021 at 4:10 PM Amit Kapila
wrote:
> > > > >
> > > > > On Tue, Aug 3, 2021 at 8:38 PM vignesh C
wrote:
> > > >
> > > > > 6.
> > > > > + {PublicationSchemaRelationId, /* PUBLICATIONSCHEMAMAP */
> > > > > + PublicationSchemaPsnspcidPspubidIndexId,
> > > > > + 2,
> > > > > + {
> > > > > + Anum_pg_publication_schema_psnspcid,
> > > > > + Anum_pg_publication_schema_pspubid,
> > > > > + 0,
> > > > > + 0
> > > > > + },
> > > > >
> > > > > Why don't we keep pubid as the first column in this index?
> > > >
> > > > I wanted to keep it similar to PUBLICATIONRELMAP, should we keep it
as
> > > > it is, thoughts?
> > > >
> > >
> > > Okay, I see your point. I think for PUBLICATIONRELMAP, we need it
> > > because it is searched using the only relid in
> > > GetRelationPublications, so, similarly, in the patch, you are using
> > > schema_oid in GetSchemaPublications, so probably that will help. I was
> > > wondering why you haven't directly used the cache in
> > > GetSchemaPublications similar to GetRelationPublications?
> >
> > Both of the approaches work, I was not sure which one is better, If
> > the approach in GetRelationPublications is better, I will change it to
> > something similar to GetRelationPublications. Thoughts?
> >
>
> I think it is better to use the cache as if we don't find the entry in
> the cache, then we will anyway search the required entry via sys table
> scan, see SearchCatCacheList.
Modified. This is handled in the v20 patch posted at [1].
I think the point I wanted to ensure was
> that a concurrent session won't blow up the entry while we are looking
> at it. How is that ensured?
The concurrency points occur at two places, Walsender session and user
session:
For Walsender process when we query the data from the cache we will get the
results based on historic snapshot. I also debugged and verified that we
get the results based on historic snapshot, if we check the cache during
our operation (insert is just before drop) we will be able to get the
dropped record from the cache as this drop occurred after our insert. And
if we query the cache after the drop, we will not get the dropped
information from the cache. So I feel our existing code is good enough
which handles the concurrency through the historic snapshot.
For user sessions, user session checks for replica identity for
update/delete operations. To prevent concurrency issues, when schema is
added to the publication, the rel cache invalidation happens in
publication_add_schema by calling InvalidatePublicationRels, similarly when
a schema is dropped from the publication, the rel cache invalidation is
handled in RemovePublicationSchemaById by calling
InvalidatePublicationRels. Once the invalidation happens it will check the
system tables again before deciding. I felt this rel cache invalidation
will prevent concurrency issues.
[1] -
https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com
Regards,
Vignesh
Re: Added schema level support for publication.
On Fri, Aug 6, 2021 at 4:02 PM Amit Kapila wrote: > > On Fri, Aug 6, 2021 at 2:16 PM vignesh C wrote: > > > > On Thu, Aug 5, 2021 at 3:54 PM Amit Kapila wrote: > > > > > > > > > Few more comments: > > > === > > > 1. Do we need the previous column 'puballtables' after adding pubtype > > > or pubkind in pg_publication? Removed puballtables, this is handled as part of v20 patch attached at [1]. [1] - https://www.postgresql.org/message-id/CALDaNm00X9SQBokUTy1OxN1Sa2DFsK8rg8j_wLgc-7ZuKcuh0Q%40mail.gmail.com Regards, Vignesh
Re: Added schema level support for publication.
Peter Eisentraut writes: > I think the strict separation between publication-for-tables vs. > publication-for-schemas is a mistake. Why can't I have a publication > that publishes tables t1, t2, t3, *and* schemas s1, s2, s3. Also note > that we have a pending patch to add sequences support to logical > replication. So eventually, a publication will be able to contain a > bunch of different objects of different kinds. This seems like it's going to create a mess, because the meaning of "include schema S" will change over time as we add more features. That is, with the present patch (I suppose, haven't read it) we have "schema S" meaning "publish all tables in schema S". When that other patch lands, presumably that same publication definition would start meaning "publish all tables and sequences in schema S". And a few years down the road it might start meaning something else again. That sounds like the sort of potentially app-breaking change that we don't like to make. We could avoid that bug-in-waiting if the syntax were more like "FOR ALL TABLES IN SCHEMA s", which could later extend to "FOR ALL SEQUENCES IN SCHEMA s", etc. This is then a very clean intermediate step between publishing one table and "FOR ALL TABLES" without a schema limitation. I tend to agree that a single publication should be able to incorporate any of these options. regards, tom lane
Re: when the startup process doesn't (logging startup delays)
Should this feature distinguish between crash recovery and archive recovery on a hot standby ? Otherwise the standby will display this all the time. 2021-08-14 16:13:33.139 CDT startup[11741] LOG: redo in progress, elapsed time: 124.42 s, current LSN: 0/EEE2100 If so, I think maybe you'd check !InArchiveRecovery (but until Robert finishes cleanup of xlog.c variables, I can't say that with much confidence).
Re: What are exactly bootstrap processes, auxiliary processes, standalone backends, normal backends(user sessions)?
On Sat, Jul 17, 2021 at 10:45:52AM -0400, Alvaro Herrera wrote: > On 2021-Jul-17, Bharath Rupireddy wrote: > > On Thu, Jul 15, 2021 at 8:17 PM Justin Pryzby wrote: > > > It sounds like something that should be in the glossary, which currently > > > refers > > > to but doesn't define "auxiliary processes". > > > > Thanks. I strongly feel that it should be documented somewhere. I will > > be happy if someone with a clear idea about these various processes > > does it. > > Auxiliary process I elaborated on your definition and added here. https://commitfest.postgresql.org/34/3285/ >From 9855c0d186251407c868fe0b0d93571541313f55 Mon Sep 17 00:00:00 2001 From: Justin Pryzby Date: Sat, 14 Aug 2021 18:51:37 -0500 Subject: [PATCH] glossary definitions for auxiliary processes --- doc/src/sgml/glossary.sgml | 58 ++ 1 file changed, 53 insertions(+), 5 deletions(-) diff --git a/doc/src/sgml/glossary.sgml b/doc/src/sgml/glossary.sgml index 63ff4bbdf0..881d16d6c4 100644 --- a/doc/src/sgml/glossary.sgml +++ b/doc/src/sgml/glossary.sgml @@ -123,6 +123,27 @@ + + Auxiliary process + + + Process that is in charge of a specific, hardcoded, background task for + an instance. + The auxiliary processes consist of + + the background writer, + the checkpointer, + the statistics collector, + the logger, + the startup process, + the WAL archiver. + the WAL receiver + (but not the WAL senders), and the + WAL writer. + + + + Backend (process) @@ -163,7 +184,7 @@ Background writer (process) - A process that writes dirty + An auxiliary process that writes dirty data pages from shared memory to the file system. It wakes up periodically, but works only for a short @@ -285,7 +306,7 @@ Checkpointer (process) - A specialized process responsible for executing checkpoints. + An auxiliary process that is responsible for executing checkpoints. @@ -881,7 +902,7 @@ Logger (process) - If activated, the process + An auxiliary process which (if enabled) writes information about database events into the current log file. When reaching certain time- or @@ -1452,6 +1473,16 @@ + + Startup process + + + An auxiliary process that replays WAL during crash recovery and (on + replicas) archive recovery. + + + + SQL object @@ -1514,7 +1545,7 @@ Stats collector (process) - This process collects statistical information about the + An auxiliary process that collects statistical information about the instance's activities. @@ -1856,7 +1887,8 @@ WAL archiver (process) - A process that saves copies of WAL files + An auxiliary process that saves copies of + WAL files for the purpose of creating backups or keeping replicas current. @@ -1914,6 +1946,22 @@ + + WAL receiver + + + An auxiliary process that runs on a + replica + to receive and replay WAL from the primary server. + + + + For more information, see + . + + + + WAL segment -- 2.17.0
Re: ECPG bug fix: DECALRE STATEMENT and DEALLOCATE, DESCRIBE
Michael Meskes writes: > I will commit the patch(es). Thanks. This commit appears to be responsible for new noise on stderr during check-world: $ make -s check-world >/dev/null declare.pgc:123: WARNING: connection "con2" is overwritten to "con1". declare.pgc:124: WARNING: connection "con2" is overwritten to "con1". declare.pgc:135: WARNING: connection "con2" is overwritten to "con1". Please do something about that. (1) There should be no output to stderr in the tests. Why isn't this message being caught and redirected to the normal test output file? (2) This message is both unintelligible and grammatically incorrect. regards, tom lane
