Re: [HACKERS] Hot Standby on git
On Mon, 2009-10-05 at 18:30 -0400, Heikki Linnakangas wrote: Simon Riggs wrote: On Mon, 2009-09-28 at 11:25 +0300, Heikki Linnakangas wrote: Heikki Linnakangas wrote: Per Simon's request, for the benefit of the archive, here's all the changes I've done on the patch since he posted the initial version for review for this commitfest as incremental patches. This is extracted from my git repository at git://git.postgresql.org/git/users/heikki/postgres.git. Further fixes extracted from above repository attached.. I've applied changes on all these patches apart from 0006-... which has some dependencies on earlier work I'm still looking at. Simon, you don't need to apply those patches. Just review them, and post comments or subsequent patches on top of the repository. I've applied them to the repository. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot Standby on git
On Tue, 2009-10-06 at 01:10 +0300, Heikki Linnakangas wrote: Simon Riggs wrote: We discussed briefly your change 0011-Replace-per-proc-counters-of-loggable-locks-with-per.patch. I don't see how that helps at all. The objective of lock counters was to know if we can skip acquiring an LWlock on all lock partitions. This change keeps the lock counters yet acquires the locks we were trying to avoid. This change needs some justification since it is not a bug fix. Well, the original code was buggy. One bug was found, yes, but the submitted changes are not just a bug fix, they alter the whole basic meaning of that code. But more to the point, it's a lot simpler this way, I don't see any reason why the counters should be per-process, meaning that they need to be exposed in the pgproc structs or procarray.c. There was a very clear reason for doing it that way. By putting the counters on the PGPROC structs it allows us to check the counts while we are performing the main sweep of the procarray *and* if the count is zero it allows us to *completely avoid* taking the locks. By making the lock counters private to each backend the counters themselves do not need to be protected by a lock when updated, and the fetch can be done atomically, as we do with xid. With respect, I have explained this on list at least twice previously, as well as in code comments. The point is to avoid the seqscan of the lock hash table. I presumed that's the expensive part in GetRunningTransactionLocks(). Tom Lane wrote: [ scratches head ... ] Why is hot standby messing with this sort of thing at all? It sounds like a performance optimization that should be considered separately, and *later*. Yeah, I too considered just ripping it out. Simon is worried that locking all the lock partitions and scanning the locks table can take a long time. We do that in the master, while holding both ProcArrayLock and XidGenLock in exclusive mode (hmm, why is shared not enough?), so there is some grounds for worry. OTOH, it's only done once per checkpoint. I could live with ripping it out, but what we have now doesn't make sense, to me. -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot Standby on git
On Sun, 2009-09-27 at 13:57 +0300, Heikki Linnakangas wrote: Per Simon's request, for the benefit of the archive, here's all the changes I've done on the patch since he posted the initial version for review for this commitfest as incremental patches. This is extracted from my git repository at git://git.postgresql.org/git/users/heikki/postgres.git. There were 16 change patches included in this post. I have applied 14 of them, almost all without any changes to comment or code. I've fixed 2 bugs and made changes where XXX comments were left in code. That leaves 0011-locks... discussed on another part of this thread and patch 0005-Include-information... which I'm discussing here. It's a huge set of changes, all of which is just refactoring, none of which is a bug fix of any kind. The refactoring does sound reasonable, but is really fairly minor. I feel we should defer this change for the future to allow us to stabilise the current patch. I do understand the need for refactoring, but if we refactor everything touched by Hot Standby then we will simply touch more code and trigger more bugs etc.. This is especially true with prepared transactions, which aren't well understood and not covered by as many tests. --- src/backend/access/transam/twophase.c | 96 +--- src/backend/access/transam/twophase_rmgr.c | 13 src/backend/access/transam/xact.c | 64 +++ src/backend/access/transam/xlog.c |5 +- src/backend/commands/discard.c |3 +- src/backend/commands/sequence.c|3 + src/backend/storage/lmgr/lock.c|5 +- src/backend/tcop/postgres.c|1 + src/backend/utils/cache/inval.c| 90 ++ src/include/access/subtrans.h |3 - src/include/access/twophase.h |2 - src/include/access/twophase_rmgr.h |6 +- src/include/access/xact.h |6 -- src/include/miscadmin.h|7 -- src/include/storage/proc.h |7 +- src/include/utils/inval.h |7 ++ 16 files changed, 108 insertions(+), 210 deletions(-) -- Simon Riggs www.2ndQuadrant.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Privileges and inheritance
Was it uncertain what I wanted to say for the proposition? In my understanding, here are two different perspectives to handle table inheritances. The one handles the inherited definitions of child tables as a part of the child table. The current implementation uses this perspective. Thus, it checks user's privilege on all the children when he tries to select data from the parent table. In constrast, it does not need to check permission on the parent tables, when he tries to select data from the child table, because the child table does not contain any part of the parent table. The other is newly proposed one. It handles the inherited definitions of child tables as a part of parent table. Thus, it does not need to check user's privilege on the children when he tries to select data from the parent table, because this query can select only columns inherited from the parent table and we can consider these are a part of the parent table in this perspective. In constrast, if we consider inherited columns are a part of the parent tables, it should check permission on the parent relation and columns when he tries to select data from the child table. If a table tbl_c(c int) inherits a table tbl_p(a int, b int), ... The first perspective considers as follows: +--- physical location of the data | --V---+---+ tbl_p | data stored | | within tbl_p | --+---+---+ tbl_c | data stored | |within tbl_c | --+---+---+---| | a | b | c | In this perspective, SELECT * FROM tbl_p accesses data within both of tbl_p and tbl_c, so it needs to check privileges to tbl_p and tbl_c. But SELECT * FROM tbl_c does not see any data within tbl_p. The later perspective considers as follows: +--- physical location of the data | --V---+---+ tbl_p | | +- data stored within tbl_c | | | --+ data stored +---|---+ tbl_c | within tbl_p | V | | | | --+---+---+---+ | a | b | c | In this perspective, SELECT * FROM tbl_p access data within only tbl_p, so it does not needs to check privilege on the tbl_c. But, SELECT * FROM tbl_c also means accesses data within both of tbl_p and tbl_c concurrently in this perspective, so I think it is quite natural to check privileges to both of tbl_p and tbl_c. In my understanding, your proposition enables DBA to choose which perspective can be applied on his system. It seems to me quite cool. However, the behavior when we select from a child table seems to me inconsistent. If it does not check privileges on the parent table when selecting from the child, it means we can adopt different perspectives concurrently. Am I missing something? I'm still unclear which perspective does it tries to provide. Thanks, BTW, I uses the term of perspective to mean point of view or philosophy. Is it confusable? Is there any more appropriate terms? KaiGai Kohei wrote: Peter Eisentraut wrote: On Mon, 2009-10-05 at 16:27 +0900, KaiGai Kohei wrote: CREATE TABLE tbl_p (int a, int b); CREATE TABLE tbl_c (int x) INHERITS(tbl_p); SELECT a,b FROM tbl_p; -- It selects data from only tbl_p. It is reasonable to bypass checks on tbl_c. SELECT b,x FROM tbl_c; -- It selects data from tbl_p and tbl_c concurrently, if we consider the inherited columns are a part of the parent table. I think you need to distinguish between the definition of columns and the data in the columns. tbl_c has inherited the definition of the columns from tbl_p, but the data is part of tbl_c, not tbl_p. So there is not reason for this second query to ask tbl_p for permission, because it does not touch data in tbl_p at all. Yes, I can understand the second query selects data stored within only tbl_c in this case, not tbl_p, even if tbl_c inherits its definitions from the parent. However, this perspective may be inconsistent to the idea to bypass permission checks on the child (tbl_c) when we select data from the parent (tbl_p), because the first query also fetches data stored within the tbl_c, not only the tbl_p. IMO, if we adopt the perspective which considers the access control depends on the physical location, the current implementation works fine. However, this idea proposed a different perspective. It allows to bypass permission checks on the child tables, because the child has identical definition with its parent and these are a part of the parent table. If so, I think this perspective should be ensured without any exception. BTW, I basically think this perspective change is better. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to
Re: [HACKERS] Streaming Replication patch for CommitFest 2009-09
Hi,
On Mon, Sep 21, 2009 at 4:51 PM, Heikki Linnakangas
heikki.linnakan...@enterprisedb.com wrote:
I've pushed that to 'replication-orig' branch in my git
repository, attached is the same as a diff against your SR_0914.patch.
The following changes about crossing a xlogid boundary seem wrong,
which would break the management of some XLOG positions.
! /* Update state for read */
! tmp = recptr.xrecoff + byteswritten;
! if (tmp recptr.xrecoff)
! recptr.xlogid++; /* overflow */
! recptr.xrecoff = tmp;
! endptr.xrecoff += MAX_SEND_SIZE;
! if(endptr.xrecoff startptr.xrecoff)
! endptr.xlogid++; /* xrecoff overflowed */
! if (endptr.xlogid != startptr.xlogid)
{
! Assert(endptr.xlogid == startptr.xlogid + 1);
! nbytes = (0x - endptr.xrecoff) +
startptr.xrecoff;
! }
The size of a logical XLOG file is 0xff00. So, even if xrecoff has
not been overflowed yet, we might need to cross a xlogid boundary.
The xrecoff should be compared with XLogFileSize, I think. Can I fix those?
Regards,
--
Fujii Masao
NIPPON TELEGRAPH AND TELEPHONE CORPORATION
NTT Open Source Software Center
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unicode UTF-8 table formatting for psql text output
On Mon, Oct 05, 2009 at 04:32:08PM -0400, Tom Lane wrote:
Roger Leigh rle...@codelibre.net writes:
On Sun, Oct 04, 2009 at 11:22:27PM +0300, Peter Eisentraut wrote:
Elsewhere in the psql code, notably in mbprint.c, we make the decision
on whether to apply certain Unicode-aware processing based on whether
the client encoding is UTF8. The same should be done here.
There is a patch somewhere in the pipeline that would automatically set
the psql client encoding to whatever the locale says, but until that is
done, the client encoding should be the sole setting that rules what
kind of character set processing is done on the client side.
OK, that makes sense to a certain extent. However, the characters
used to draw the table lines are not really that related to the
client encoding for data sent from the database (IMHO).
Huh? The data *in* the table is going to be in the client_encoding, and
psql contains no mechanisms that would translate it to something else.
Surrounding it with decoration in a different encoding is just a recipe
for breakage.
Ah, I was under the mistaken assumption that this was iconv()ed or
otherwise translated for correct display. In that case, I'll leave
the patch as is (using the client encoding for table lines).
I've attached an updated copy of the patch (it just removes the
now unneeded langinfo.h header).
Regards,
Roger
--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`-GPG Public Key: 0x25BFB848 Please GPG sign your mail.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 85e9375..cd1c137 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1690,6 +1690,54 @@ lo_import 152801
/varlistentry
varlistentry
+ termliterallinestyle/literal/term
+ listitem
+ para
+ Sets the line drawing style of text table output to one
+ of literalascii/literal, or literalutf8/literal.
+ Unique abbreviations are allowed. (That would mean one
+ letter is enough.) literalutf8/literal will be selected
+ by default if supported by your locale,
+ otherwise literalascii/literal will be used.
+ /para
+
+ para
+ Query result tables are displayed as text for some output
+ formats (literalunaligned/literal,
+ literalaligned/literal, and literalwrapped/literal
+ formats). The tables are drawn using characters of the
+ user's locale character set. By
+ default, acronymASCII/acronym characters will be used,
+ which will display correctly in all locales. However, if
+ the user is using a locale with a acronymUTF-8/acronym
+ character set, the default will be to
+ use acronymUTF-8/acronym box drawing characters in place
+ of ASCII punctuation to display more readable tables.
+ /para
+
+ para
+ This option is useful for overriding the default line
+ style, for example to force the use of
+ only acronymASCII/acronym characters when extended
+ character sets such as acronymUTF-8/acronym are
+ inappropriate. This might be the case if preserving output
+ compatibility with older psql versions is important (prior
+ to 8.5.0).
+ /para
+
+ para
+ quoteUTF8/quote use Unicode acronymUTF-8/acronym box
+ drawing characters.
+ /para
+
+ para
+ quoteASCII/quote use plain acronymASCII/acronym characters.
+ /para
+
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteralcolumns/literal/term
listitem
para
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index da57eb4..be2b60b 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -33,6 +33,11 @@
#include openssl/ssl.h
#endif
+#include locale.h
+#ifdef HAVE_LANGINFO_H
+#include langinfo.h
+#endif
+
#include portability/instr_time.h
#include libpq-fe.h
@@ -46,6 +51,7 @@
#include input.h
#include large_obj.h
#include mainloop.h
+#include mbprint.h
#include print.h
#include psqlscan.h
#include settings.h
@@ -596,6 +602,14 @@ exec_command(const char *cmd,
/* save encoding info into psql internal data */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
+if (!pset.popt.topt.line_style_set)
+{
+ if (pset.encoding == get_utf8_id())
+ pset.popt.topt.line_style = utf8format;
+ else
+ pset.popt.topt.line_style = asciiformat;
+}
+
SetVariable(pset.vars, ENCODING,
pg_encoding_to_char(pset.encoding));
}
@@ -1430,8 +1444,19 @@ void
SyncVariables(void)
{
/* get stuff from connection */
+#if (defined(HAVE_LANGINFO_H) defined(CODESET))
+ if (PQsetClientEncoding(pset.db,
[HACKERS] Patch: create or replace language
Hello,
following this old discussion:
http://archives.postgresql.org/pgsql-patches/2008-03/msg00402.php
i modifies the patch to use the CREATE [OR REPLACE] LANGUAGE syntax.
If the patch is ok, i will add the documentation too.
Kind regards
--
Andreas 'ads' Scherbaum
German PostgreSQL User Group
European PostgreSQL User Group - Board of Directors
Volunteer Regional Contact, Germany - PostgreSQL Project
PGDay.eu 2009 in Paris, Nov. 6/7, http://www.pgday.eu/
Index: src/backend/commands/proclang.c
===
RCS file: /projects/cvsroot/pgsql/src/backend/commands/proclang.c,v
retrieving revision 1.87
diff -r1.87 proclang.c
84,86c84,94
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg(language \%s\ already exists, languageName)));
---
if (!stmt-replace)
ereport(ERROR,
(errcode(ERRCODE_DUPLICATE_OBJECT),
errmsg(language \%s\ already exists, languageName)));
else
{
ereport(NOTICE,
(errmsg(language \%s\ already exists, skipping, languageName)));
return;
}
Index: src/backend/parser/gram.y
===
RCS file: /projects/cvsroot/pgsql/src/backend/parser/gram.y,v
retrieving revision 2.679
diff -r2.679 gram.y
2768c2768
CREATE opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
---
CREATE opt_or_replace opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
2771c2771,2772
n-plname = $5;
---
n-replace = $2;
n-plname = $6;
2779c2780
| CREATE opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
---
| CREATE opt_or_replace opt_trusted opt_procedural LANGUAGE ColId_or_Sconst
2783,2787c2784,2789
n-plname = $5;
n-plhandler = $7;
n-plinline = $8;
n-plvalidator = $9;
n-pltrusted = $2;
---
n-replace = $2;
n-plname = $6;
n-plhandler = $8;
n-plinline = $9;
n-plvalidator = $10;
n-pltrusted = $3;
Index: src/include/nodes/parsenodes.h
===
RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v
retrieving revision 1.402
diff -r1.402 parsenodes.h
1570a1571
bool replace; /* T = replace if already exists */
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Encoding issues in console and eventlog on win32
2009/9/15 Itagaki Takahiro itagaki.takah...@oss.ntt.co.jp: Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Can't we use MultiByteToWideChar() to convert directly to the required encoding, avoiding the double conversion? Here is an updated version of the patch. I use direct conversion in pgwin32_toUTF16() if a corresponding codepage is available. If not available, I still use double conversion. Now pgwin32_toUTF16() is exported from mbutil.c. I used the function in following parts, although the main target of the patch is eventlog. * WriteConsoleW() - write unredirected stderr log. * ReportEventW() - write evenlog. * CreateFileW() - open non-ascii filename (ex. COPY TO/FROM 'mb-path'). This approach is only available for Windows because any other platform don't support locale-independent and wide-character-based system calls. Other platforms require a different approach, but even then we'd still better have win32-specific routines because UTF16 is the native encoding in Windows. I did a quick check of this, and here are the things I would like to have changed: First of all, the change to port/open.c seems to be unrelated to the rest, and should be a separate patch, correct? I'm sure there's a usecase for it, but it's not actually included in the patches description, so I assume this was a mistake? Per your own comments earlier, and in the code, what will happen if pg_do_encoding_conversion() calls ereport()? Didn't you say we need a non-throwing version of it? pgwin32_toUTF16() needs error checking on the API calls, and needs to do something reasonable if it fails. For example, it can fail because of out of memory error. I suggest just returning the error code in some way in that case, and have the callers fall back to logging in the incorrect encoding - in a lot of cases that will produce an at least partially readable message. A second message should also be logged saying that the conversion failed - this needs to be done directly with the eventlog API functions and not ereport, so we don't end up in infinite recursion. The encoding_to_codepage array needs to go in encnames.c, where other such tables are. Perhaps it can even be integrated in pg_enc2name_tbl as a separate field? I don't have the time to clean this up right now, so if you have, please do so and resubmit. If not, I can clean it up later and apply. -- Magnus Hagander Me: http://www.hagander.net/ Work: http://www.redpill-linpro.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
KaiGai Kohei napsal(a): I tried to check the default ACL behavior. It works for me fine, good, but ... postgres= SELECT * INTO t3 FROM t1; SELECT postgres= SELECT * FROM t3; a | b ---+- 1 | aaa 2 | bbb (2 rows) postgres= INSERT INTO t3 VALUES (3,'ccc'); ERROR: permission denied for relation t3 In this case, the new table t3 is created with the default ACL which does not allow to insert any values by the owner of the relation. SELECT INTO does not check ACL_INSERT on the newly created tables, because we had been able to assume the table owner always has privilege to insert values into the new table. So, OpenIntoRel() didn't check this obvious privilege. But the default ACL feature breaks this assumption. The table owner may not have privilege to insert values into new tables. So, it is necessary to put actual access controls on the OpenIntoRel(). That's strange behavior I agree. However I don't see how default ACLs changed it in any way, owner could REVOKE his privileges before. -- Regards Petr Jelinek (PJMODOS) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek napsal(a): Tom Lane napsal(a): Petr Jelinek pjmo...@pjmodos.net mailto:pjmo...@pjmodos.net writes: Tom Lane napsal(a): One thing that seems like it's likely to be an annoyance in practice is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl entries for a role to be dropped. Yeah I am not happy about this either but there is not much we can do about it. Btw I think in the version I sent in REASSIGN OWNED acted as DROP OWNED for default ACLs. IIRC it just threw a warning, which didn't seem tremendously useful to me. Oh did it ? Then I must have discarded that idea for some reason. I probably didn't want to be too pushy there. Now I remember why - consistency with ACLs on object. REASSIGN OWNED does not drop any GRANTed ACLs on any object, so it seemed appropriate to only drop default ACLs in DROP OWNED BY along with ACLs on objects. -- Regards Petr Jelinek (PJMODOS)
Re: [HACKERS] Streaming Replication patch for CommitFest 2009-09
Fujii Masao escribió: On Thu, Sep 17, 2009 at 5:08 PM, Heikki Linnakangas heikki.linnakan...@enterprisedb.com wrote: Walreceiver is really a slave to the startup process. The startup process decides when it's launched, and it's the startup process that then waits for it to advance. But the way it's set up at the moment, the startup process needs to ask the postmaster to start it up, and it doesn't look very robust to me. For example, if launching walreceiver fails for some reason, startup process will just hang waiting for it. I changed the postmaster to report the failure of fork of the walreceiver to the startup process by resetting WalRcv-in_progress, which prevents the startup process from getting stuck when launching walreceiver fails. http://archives.postgresql.org/pgsql-hackers/2009-09/msg01996.php Do you have another concern about the robustness? If yes, I'll address that. Hmm. Without looking at the patch at all, this seems similar to how autovacuum does things: autovac launcher signals postmaster that a worker needs to be started. Postmaster proceeds to fork a worker. This could obviously fail for a lot of reasons. Now, there is code in place to notify the user when forking fails, and this is seen on the wild quite a bit more than one would like :-( I think it would be a good idea to have a retry mechanism in the walreceiver startup mechanism so that recovery does not get stuck due to transient problems. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Privileges and inheritance
KaiGai Kohei kai...@ak.jp.nec.com writes: Was it uncertain what I wanted to say for the proposition? No: it's that nobody sees any particular value in making a fundamental restructuring of the permissions system like that. What you propose would be far harder/more complicated to implement, to understand, or to use. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek wrote: KaiGai Kohei napsal(a): I tried to check the default ACL behavior. It works for me fine, good, but ... postgres= SELECT * INTO t3 FROM t1; SELECT postgres= SELECT * FROM t3; a | b ---+- 1 | aaa 2 | bbb (2 rows) postgres= INSERT INTO t3 VALUES (3,'ccc'); ERROR: permission denied for relation t3 In this case, the new table t3 is created with the default ACL which does not allow to insert any values by the owner of the relation. SELECT INTO does not check ACL_INSERT on the newly created tables, because we had been able to assume the table owner always has privilege to insert values into the new table. So, OpenIntoRel() didn't check this obvious privilege. But the default ACL feature breaks this assumption. The table owner may not have privilege to insert values into new tables. So, it is necessary to put actual access controls on the OpenIntoRel(). That's strange behavior I agree. However I don't see how default ACLs changed it in any way, owner could REVOKE his privileges before. I don't think the default ACL feature should do something ad-hoc here. Is there anything necessary more than adding permission checks to insert values into the new table? Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
Andreas 'ads' Scherbaum adsm...@wars-nicht.de writes: following this old discussion: http://archives.postgresql.org/pgsql-patches/2008-03/msg00402.php i modifies the patch to use the CREATE [OR REPLACE] LANGUAGE syntax. This is not an OR REPLACE operation, because it doesn't replace the existing definition. What you've got here is a CREATE IF NOT EXISTS implementation that arbitrarily uses the other syntax. The point of the previous discussion was summed up here: http://archives.postgresql.org/pgsql-patches/2008-03/msg00416.php namely that CREATE OR REPLACE should leave the object having the properties specified in the command. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
Andreas 'ads' Scherbaum wrote: Hello, following this old discussion: http://archives.postgresql.org/pgsql-patches/2008-03/msg00402.php i modifies the patch to use the CREATE [OR REPLACE] LANGUAGE syntax. If the patch is ok, i will add the documentation too. Please send a context diff (however much ED IS THE STANDARD!!! TEXT EDITOR, we don't like its patches here). Note that you probably missed updates to other functions touching the node to which you add the boolean. Also, per Tom's followup, Index: src/include/nodes/parsenodes.h === RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v retrieving revision 1.402 diff -r1.402 parsenodes.h 1570a1571 boolreplace;/* T = replace if already exists */ this comment needs fixed. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek escribió: Petr Jelinek napsal(a): Tom Lane napsal(a): Petr Jelinek pjmo...@pjmodos.net mailto:pjmo...@pjmodos.net writes: Tom Lane napsal(a): One thing that seems like it's likely to be an annoyance in practice is the need to explicitly do DROP OWNED BY to get rid of pg_default_acl entries for a role to be dropped. Yeah I am not happy about this either but there is not much we can do about it. Btw I think in the version I sent in REASSIGN OWNED acted as DROP OWNED for default ACLs. IIRC it just threw a warning, which didn't seem tremendously useful to me. Oh did it ? Then I must have discarded that idea for some reason. I probably didn't want to be too pushy there. Now I remember why - consistency with ACLs on object. REASSIGN OWNED does not drop any GRANTed ACLs on any object, so it seemed appropriate to only drop default ACLs in DROP OWNED BY along with ACLs on objects. That seems reasonable to me too ... -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] moving system catalogs to another tablespace
On Mon, Oct 5, 2009 at 10:26 PM, Tom Lane t...@sss.pgh.pa.us wrote: Jaime Casanova jcasa...@systemguards.com.ec writes: seems like the original idea was to forbid this in all system catalogs except pg_largeobject, what happen then? Nothing ... nobody got around to doing anything about it. ah! well, having slept a while my thinking is a little bit more sane... now i think that what Euler shows me [1] is a fair compromise (this is to allow this only when in standalone mode with system catalogs allowed) otherwise we will have diferent behaviour for specific (random) catalogs... [1] http://listas.postgresql.org.br/pipermail/pgbr-geral/2009-March/014374.html -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
On Tue, Oct 6, 2009 at 10:24 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Andreas 'ads' Scherbaum wrote: Hello, following this old discussion: http://archives.postgresql.org/pgsql-patches/2008-03/msg00402.php i modifies the patch to use the CREATE [OR REPLACE] LANGUAGE syntax. If the patch is ok, i will add the documentation too. Please send a context diff (however much ED IS THE STANDARD!!! TEXT EDITOR, we don't like its patches here). Note that you probably missed updates to other functions touching the node to which you add the boolean. Also, per Tom's followup, Index: src/include/nodes/parsenodes.h === RCS file: /projects/cvsroot/pgsql/src/include/nodes/parsenodes.h,v retrieving revision 1.402 diff -r1.402 parsenodes.h 1570a1571 bool replace; /* T = replace if already exists */ this comment needs fixed. Maybe I'm out of line to say this, but it seems to me that we should not even be looking at newly-submitted patches at this point. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Petr Jelinek pjmo...@pjmodos.net writes: KaiGai Kohei napsal(a): SELECT INTO does not check ACL_INSERT on the newly created tables, because we had been able to assume the table owner always has privilege to insert values into the new table. So, OpenIntoRel() didn't check this obvious privilege. But the default ACL feature breaks this assumption. The table owner may not have privilege to insert values into new tables. So, it is necessary to put actual access controls on the OpenIntoRel(). That's strange behavior I agree. However I don't see how default ACLs changed it in any way, owner could REVOKE his privileges before. The point is that some rows got into the new table, which should have been disallowed if CREATE TABLE AS is considered to represent a CREATE followed by an INSERT. However, I disagree that this necessarily represents a bug in the permissions checks. We could perfectly well define that INSERT means the right to insert into the table *after it is created*, and that CREATE TABLE AS does not involve this right. I think that that is a reasonable and potentially useful thing to do, whereas defining it as KaiGai-san suggests would have no usefulness whatsoever. What's more, CREATE TABLE AS is specified in the SQL standard, and I see nothing in the standard mentioning that it requires INSERT privilege. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
Robert Haas escribió: On Tue, Oct 6, 2009 at 10:24 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: this comment needs fixed. Maybe I'm out of line to say this, but it seems to me that we should not even be looking at newly-submitted patches at this point. Yeah, it was so short I didn't see any point in not commenting. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] moving system catalogs to another tablespace
Jaime Casanova wrote: now i think that what Euler shows me [1] is a fair compromise (this is to allow this only when in standalone mode with system catalogs allowed) otherwise we will have diferent behaviour for specific (random) catalogs... [1] http://listas.postgresql.org.br/pipermail/pgbr-geral/2009-March/014374.html Hmm, I don't necessarily agree that having the system effectively shut down for the duration of the pg_largeobject move is a good idea. I don't agree that pg_largeobject is a random catalog either -- it is, in fact, the only catalog in which an interesting size is to be expected. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Privileges and inheritance
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: Was it uncertain what I wanted to say for the proposition? No: it's that nobody sees any particular value in making a fundamental restructuring of the permissions system like that. What you propose would be far harder/more complicated to implement, to understand, or to use. Yes, I agree. If we actually implement the new perspective perfectly, it needs complex implementation due to the differences from physical structure. However, it seems to me the current proposition tries to adopt a mixed perspectives depending on the situation Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
Robert Haas robertmh...@gmail.com writes: Maybe I'm out of line to say this, but it seems to me that we should not even be looking at newly-submitted patches at this point. It would have been more work to put it in the queue to reject later than to reject it now ;-). Besides, this way Andreas has a chance to rewrite it into something acceptable before the next CF. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
On Tue, Oct 6, 2009 at 10:48 AM, Tom Lane t...@sss.pgh.pa.us wrote: Robert Haas robertmh...@gmail.com writes: Maybe I'm out of line to say this, but it seems to me that we should not even be looking at newly-submitted patches at this point. It would have been more work to put it in the queue to reject later than to reject it now ;-). Besides, this way Andreas has a chance to rewrite it into something acceptable before the next CF. Fair enough. ...Robert -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Hot Standby on git
On Thu, 2009-10-01 at 18:47 +0300, Heikki Linnakangas wrote:
And if you could please review the changes I've been doing, just to
make sure I haven't inadvertently introduced new bugs. That has
happened before, as you've rightfully reminded me :-).
You posted 17 patches here.
I've reviewed/applied patches 1,3,4,5,6,7,9,10,13,14,15.
That leaves me with some form of issue on 2, 5, 8, 11, 12, 16 and 17.
Sounds a lot, but out of 41 patches in total to date, I have as yet
unresolved issues with 9.
Patch 0017 has significant changes to it, so I'm posting patches here
for further discussion. Main line thought is that I agree with the
changes you wanted to make and I've added a few extra things.
Commit message from repo:
Apply 0017-Revert-changes-to-subtrans.c-and-slru.c.-Instead-cal.patch
but with heavy modifications to fix a number of bugs and make associated
changes. First, StartupSubtrans() positioned itself at oldestXid, so
that when later running transactions complete they could find no page
for them to update and crash. Second, ExtendClog() expected to be able
to write WAL during recovery and so crashed after 32768 xids. This patch
also extends the patch to cover the recently added support for starting
Hot Standby from a shutdown checkpoint, which causes some refactoring.
Various comments reworded, including allowing a lock overflow to cause a
PENDING state, just as we do with subxid overflow. Another bug was also
found, in that failing to make subtrans entries from the initial
snapshot could lead to later abort records hanging because the topxid
was not set. Code is now similar in all code paths. Sounds like a lot of
changes, but mostly subtle changes rather than lengthy ones.
It seems highly likely that you'll find an error in my changes to your
changes also, but they do pass initial testing.
--
Simon Riggs www.2ndQuadrant.com
*** a/src/backend/access/transam/clog.c
--- b/src/backend/access/transam/clog.c
***
*** 575,581 ExtendCLOG(TransactionId newestXact)
LWLockAcquire(CLogControlLock, LW_EXCLUSIVE);
/* Zero the page and make an XLOG entry about it */
! ZeroCLOGPage(pageno, true);
LWLockRelease(CLogControlLock);
}
--- 575,581
LWLockAcquire(CLogControlLock, LW_EXCLUSIVE);
/* Zero the page and make an XLOG entry about it */
! ZeroCLOGPage(pageno, !InRecovery);
LWLockRelease(CLogControlLock);
}
*** a/src/backend/access/transam/slru.c
--- b/src/backend/access/transam/slru.c
***
*** 598,605 SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
* commands to set the commit status of transactions whose bits are in
* already-truncated segments of the commit log (see notes in
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
! * where the file doesn't exist, and return zeroes instead. We also
! * return a zeroed page when seek and read fails.
*/
fd = BasicOpenFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
if (fd 0)
--- 598,604
* commands to set the commit status of transactions whose bits are in
* already-truncated segments of the commit log (see notes in
* SlruPhysicalWritePage). Hence, if we are InRecovery, allow the case
! * where the file doesn't exist, and return zeroes instead.
*/
fd = BasicOpenFile(path, O_RDWR | PG_BINARY, S_IRUSR | S_IWUSR);
if (fd 0)
***
*** 620,633 SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
if (lseek(fd, (off_t) offset, SEEK_SET) 0)
{
- if (InRecovery)
- {
- ereport(LOG,
- (errmsg(file \%s\ doesn't exist, reading as zeroes,
- path)));
- MemSet(shared-page_buffer[slotno], 0, BLCKSZ);
- return true;
- }
slru_errcause = SLRU_SEEK_FAILED;
slru_errno = errno;
close(fd);
--- 619,624
***
*** 637,650 SlruPhysicalReadPage(SlruCtl ctl, int pageno, int slotno)
errno = 0;
if (read(fd, shared-page_buffer[slotno], BLCKSZ) != BLCKSZ)
{
- if (InRecovery)
- {
- ereport(LOG,
- (errmsg(file \%s\ doesn't exist, reading as zeroes,
- path)));
- MemSet(shared-page_buffer[slotno], 0, BLCKSZ);
- return true;
- }
slru_errcause = SLRU_READ_FAILED;
slru_errno = errno;
close(fd);
--- 628,633
*** a/src/backend/access/transam/subtrans.c
--- b/src/backend/access/transam/subtrans.c
***
*** 31,37
#include access/slru.h
#include access/subtrans.h
#include access/transam.h
- #include miscadmin.h
#include pg_trace.h
#include utils/snapmgr.h
--- 31,36
***
*** 45,52
* 0x/SUBTRANS_XACTS_PER_PAGE, and segment numbering at
* 0x/SUBTRANS_XACTS_PER_PAGE/SLRU_SEGMENTS_PER_PAGE. We need take no
* explicit notice of that fact in this module, except when comparing segment
! * and page numbers in TruncateSUBTRANS (see SubTransPagePrecedes)
! * and in recovery when we do ExtendSUBTRANS.
*/
/* We need four bytes per
Re: [HACKERS] moving system catalogs to another tablespace
Alvaro Herrera alvhe...@commandprompt.com writes: I don't agree that pg_largeobject is a random catalog either -- it is, in fact, the only catalog in which an interesting size is to be expected. Yeah, I have sometimes thought that pg_largeobject shouldn't be considered a system catalog at all. It's more nearly like a toast table, ie, it's storing out of line user data. This has some interesting connections with the proposed changes for associating privilege data with large objects. The proposed meta table would certainly qualify as a system catalog still. Would there be any sense in redefining pg_largeobject as an actual toast table attached to that catalog? Probably not, or at least it wouldn't directly contribute to solving Jaime's problem. But it seems like now would be a good time to think outside the box a little bit about where we want to go with pg_largeobject. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] moving system catalogs to another tablespace
On Tue, Oct 6, 2009 at 9:43 AM, Alvaro Herrera alvhe...@commandprompt.com wrote: Jaime Casanova wrote: now i think that what Euler shows me [1] is a fair compromise (this is to allow this only when in standalone mode with system catalogs allowed) otherwise we will have diferent behaviour for specific (random) catalogs... [1] http://listas.postgresql.org.br/pipermail/pgbr-geral/2009-March/014374.html Hmm, I don't necessarily agree that having the system effectively shut down for the duration of the pg_largeobject move is a good idea. well, my thinking was that if you know how to start in standalone and know to allow system catalogs changes is more probable you did your homework and read about the dangers it implies in other catalogs... but yeah! the size of the pg_largeobject could be large enough to make this something to worry about... let me ask the opinion of the bottle of coke that is supporting me... I don't agree that pg_largeobject is a random catalog either -- it is, in fact, the only catalog in which an interesting size is to be expected. i have just read Tom's comments and yes that question was around my mind to: a system catalog that doesn't behaves like other system catalogs and in which we want different sets of permissions (see kaigai san's patch about largeobject controls in which he actually add syntax for row level permission in that catalog, something we don't have in any other place yet) is really a system catalog? -- Atentamente, Jaime Casanova Soporte y capacitación de PostgreSQL Asesoría y desarrollo de sistemas Guayaquil - Ecuador Cel. +59387171157 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] moving system catalogs to another tablespace
Jaime Casanova escreveu: i have just read Tom's comments and yes that question was around my mind to: a system catalog that doesn't behaves like other system catalogs and in which we want different sets of permissions (see kaigai san's patch about largeobject controls in which he actually add syntax for row level permission in that catalog, something we don't have in any other place yet) is really a system catalog? IMHO it's hibrid (catalog and regular table). That' why people proposed the SET TABLESPACE and ACL. -- Euler Taveira de Oliveira http://www.timbira.com/ -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] moving system catalogs to another tablespace
Hi all, On Tue, 2009-10-06 at 16:58 +0200, Tom Lane wrote: Yeah, I have sometimes thought that pg_largeobject shouldn't be considered a system catalog at all. It's more nearly like a toast table, ie, it's storing out of line user data. pg_largeobject in it's current form has serious limitations, the biggest one is that it can't have triggers, and thus it can't be replicated by trigger based replication like slony. I ended up rolling my own large object table, modeling exactly the behavior of pg_largeobject but on the client side, except I can replicate it... and a few other simple things like easily duplicating an entry from client side code, and easier control of the large object ID ranges - BTW, OID is not the best data type for a client visible primary key, then better BIGINT, oid is unsigned and in Java for example won't cleanly map to any data type (java long is twice as big as needed and int is signed and won't work for all OID values - we finally had to use long, but then BIGINT is a better match). Considering that the postgres manual says: using a user-created table's OID column as a primary key is discouraged, I don't see why use OID as the primary key for a table which can potentially outgrow the OID range. The backup is also not a special case now, it just dumps the table. I don't know what were the reasons of special casing pg_largeobject, but from a usability POV is fairly bad. Cheers, Csaba. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Largeobject access controls
KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] doc/src/sgml/Makefile versus VPATH
So isn't this still pretty broken? I notice that the clean and distclean targets still use addprefix on a lot of temporary/intermediate files that I would think get made in the build directory, not the source directory. The references to man files in srcdir in nonsql_manpage_files and adjacent macros seem a tad suspicious as well. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. Are you talking about syscache.c? I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. Thanks, -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doc/src/sgml/Makefile versus VPATH
Tom Lane wrote: So isn't this still pretty broken? I notice that the clean and distclean targets still use addprefix on a lot of temporary/intermediate files that I would think get made in the build directory, not the source directory. Yeah, I noticed that too. I'm not too sure about it, because some of those files we do want shipped in source tarballs; and they are definitely cleaned in the builddir by maintainer-clean. I didn't want to get into the detail of what's the ultimate distclean charter; my immediate problem was that make -C doc maintainer-clean was not getting rid of the stamp files and thus I was getting bit by the problem that openjade was running all the time, even after I maintainer-cleaned. By Peter's recent decree that tarballs are supposed to be built in non-vpath-builds only, I am not really sure what should actually happen here, both on build and on the various clean targets. The references to man files in srcdir in nonsql_manpage_files and adjacent macros seem a tad suspicious as well. I agree, it probably merits more investigation. -- Alvaro Herrerahttp://www.CommandPrompt.com/ PostgreSQL Replication, Consulting, Custom Development, 24x7 support -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Largeobject access controls
KaiGai Kohei escribió: I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. The point is not the size of the cache entries, but the number of them. -- Alvaro Herrerahttp://www.CommandPrompt.com/ The PostgreSQL Company - Command Prompt, Inc. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doc/src/sgml/Makefile versus VPATH
Alvaro Herrera alvhe...@commandprompt.com writes: By Peter's recent decree that tarballs are supposed to be built in non-vpath-builds only, I am not really sure what should actually happen here, both on build and on the various clean targets. I think that that means the Makefile can just assume that *every* built file is built in the current directory, and $(srcdir) should only be applied to non-derived files. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Patch: create or replace language
--On 6. Oktober 2009 10:13:54 -0400 Tom Lane t...@sss.pgh.pa.us wrote: http://archives.postgresql.org/pgsql-patches/2008-03/msg00416.php namely that CREATE OR REPLACE should leave the object having the properties specified in the command. Maybe when implementing this, it can be worth to keep an eye on ALTER LANGUAGE too ? -- Thanks Bernd -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Largeobject access controls
Alvaro Herrera wrote: KaiGai Kohei escribió: I added a syscache entry for pg_largeobject_metadata, not pg_largeobject which contains data chunks. The pg_largeobject_metadata is a smaller catalog than most of system catalogs, such as pg_class. The point is not the size of the cache entries, but the number of them. If so, I'll replace any routines which use LARGEOBJECTOID cache. Please wait for the revised patch. Is there any other comment? -- KaiGai Kohei kai...@kaigai.gr.jp -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Unicode UTF-8 table formatting for psql text output
On Tue, Oct 06, 2009 at 10:44:27AM +0100, Roger Leigh wrote:
On Mon, Oct 05, 2009 at 04:32:08PM -0400, Tom Lane wrote:
Roger Leigh rle...@codelibre.net writes:
On Sun, Oct 04, 2009 at 11:22:27PM +0300, Peter Eisentraut wrote:
Elsewhere in the psql code, notably in mbprint.c, we make the decision
on whether to apply certain Unicode-aware processing based on whether
the client encoding is UTF8. The same should be done here.
There is a patch somewhere in the pipeline that would automatically set
the psql client encoding to whatever the locale says, but until that is
done, the client encoding should be the sole setting that rules what
kind of character set processing is done on the client side.
OK, that makes sense to a certain extent. However, the characters
used to draw the table lines are not really that related to the
client encoding for data sent from the database (IMHO).
Huh? The data *in* the table is going to be in the client_encoding, and
psql contains no mechanisms that would translate it to something else.
Surrounding it with decoration in a different encoding is just a recipe
for breakage.
Ah, I was under the mistaken assumption that this was iconv()ed or
otherwise translated for correct display. In that case, I'll leave
the patch as is (using the client encoding for table lines).
I've attached an updated copy of the patch (it just removes the
now unneeded langinfo.h header).
This patch included a bit of code not intended for inclusion
(setting of client encoding based on locale), which the attached
(and hopefully final!) revision of the patch excludes.
Regards,
Roger
--
.''`. Roger Leigh
: :' : Debian GNU/Linux http://people.debian.org/~rleigh/
`. `' Printing on GNU/Linux? http://gutenprint.sourceforge.net/
`-GPG Public Key: 0x25BFB848 Please GPG sign your mail.
diff --git a/doc/src/sgml/ref/psql-ref.sgml b/doc/src/sgml/ref/psql-ref.sgml
index 85e9375..cd1c137 100644
--- a/doc/src/sgml/ref/psql-ref.sgml
+++ b/doc/src/sgml/ref/psql-ref.sgml
@@ -1690,6 +1690,54 @@ lo_import 152801
/varlistentry
varlistentry
+ termliterallinestyle/literal/term
+ listitem
+ para
+ Sets the line drawing style of text table output to one
+ of literalascii/literal, or literalutf8/literal.
+ Unique abbreviations are allowed. (That would mean one
+ letter is enough.) literalutf8/literal will be selected
+ by default if supported by your locale,
+ otherwise literalascii/literal will be used.
+ /para
+
+ para
+ Query result tables are displayed as text for some output
+ formats (literalunaligned/literal,
+ literalaligned/literal, and literalwrapped/literal
+ formats). The tables are drawn using characters of the
+ user's locale character set. By
+ default, acronymASCII/acronym characters will be used,
+ which will display correctly in all locales. However, if
+ the user is using a locale with a acronymUTF-8/acronym
+ character set, the default will be to
+ use acronymUTF-8/acronym box drawing characters in place
+ of ASCII punctuation to display more readable tables.
+ /para
+
+ para
+ This option is useful for overriding the default line
+ style, for example to force the use of
+ only acronymASCII/acronym characters when extended
+ character sets such as acronymUTF-8/acronym are
+ inappropriate. This might be the case if preserving output
+ compatibility with older psql versions is important (prior
+ to 8.5.0).
+ /para
+
+ para
+ quoteUTF8/quote use Unicode acronymUTF-8/acronym box
+ drawing characters.
+ /para
+
+ para
+ quoteASCII/quote use plain acronymASCII/acronym characters.
+ /para
+
+ /listitem
+ /varlistentry
+
+ varlistentry
termliteralcolumns/literal/term
listitem
para
diff --git a/src/bin/psql/command.c b/src/bin/psql/command.c
index da57eb4..223f11c 100644
--- a/src/bin/psql/command.c
+++ b/src/bin/psql/command.c
@@ -46,6 +46,7 @@
#include input.h
#include large_obj.h
#include mainloop.h
+#include mbprint.h
#include print.h
#include psqlscan.h
#include settings.h
@@ -596,6 +597,14 @@ exec_command(const char *cmd,
/* save encoding info into psql internal data */
pset.encoding = PQclientEncoding(pset.db);
pset.popt.topt.encoding = pset.encoding;
+if (!pset.popt.topt.line_style_set)
+{
+ if (pset.encoding == get_utf8_id())
+ pset.popt.topt.line_style = utf8format;
+ else
+ pset.popt.topt.line_style = asciiformat;
+}
+
SetVariable(pset.vars, ENCODING,
pg_encoding_to_char(pset.encoding));
}
@@ -1432,6 +1441,13 @@ SyncVariables(void)
/* get stuff from connection */
Re: [HACKERS] doc/src/sgml/Makefile versus VPATH
On Tue, 2009-10-06 at 12:56 -0400, Tom Lane wrote: Alvaro Herrera alvhe...@commandprompt.com writes: By Peter's recent decree that tarballs are supposed to be built in non-vpath-builds only, I am not really sure what should actually happen here, both on build and on the various clean targets. I think that that means the Makefile can just assume that *every* built file is built in the current directory, and $(srcdir) should only be applied to non-derived files. More or less, except when you are installing, you need to look in both places for files to install (and preferably avoid installing both, I think). -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] contrib/plantuner - enable PostgreSQL planner hints
Hi there, this is an announcement of our new contribution module for PostgreSQL - Plantuner - enable planner hints (http://www.sai.msu.su/~megera/wiki/plantuner). Example: =# LOAD 'plantuner'; =# create table test(id int); =# create index id_idx on test(id); =# create index id_idx2 on test(id); =# \d test Table public.test Column | Type | Modifiers +-+--- id | integer | Indexes: id_idx btree (id) id_idx2 btree (id) =# explain select id from test where id=1; QUERY PLAN --- Bitmap Heap Scan on test (cost=4.34..15.03 rows=12 width=4) Recheck Cond: (id = 1) - Bitmap Index Scan on id_idx2 (cost=0.00..4.34 rows=12 width=0) Index Cond: (id = 1) (4 rows) =# set enable_seqscan=off; =# set plantuner.forbid_index='id_idx2'; =# explain select id from test where id=1; QUERY PLAN -- Bitmap Heap Scan on test (cost=4.34..15.03 rows=12 width=4) Recheck Cond: (id = 1) - Bitmap Index Scan on id_idx (cost=0.00..4.34 rows=12 width=0) Index Cond: (id = 1) (4 rows) =# set plantuner.forbid_index='id_idx2,id_idx'; =# explain select id from test where id=1; QUERY PLAN - Seq Scan on test (cost=100.00..140.00 rows=12 width=4) Filter: (id = 1) (2 rows) Regards, Oleg _ Oleg Bartunov, Research Scientist, Head of AstroNet (www.astronet.ru), Sternberg Astronomical Institute, Moscow University, Russia Internet: o...@sai.msu.su, http://www.sai.msu.su/~megera/ phone: +007(495)939-16-83, +007(495)939-23-83 -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] doc/src/sgml/Makefile versus VPATH
On Tue, 2009-10-06 at 12:24 -0400, Tom Lane wrote: So isn't this still pretty broken? I notice that the clean and distclean targets still use addprefix on a lot of temporary/intermediate files that I would think get made in the build directory, not the source directory. Yeah, those rules have evolved a bit. Basically, everything should be in clean except the bits that go into the tarball, namely the final man and html builds. I have committed some fixes. (It almost looks like 8.4 again now.) The references to man files in srcdir in nonsql_manpage_files and adjacent macros seem a tad suspicious as well. Indeed. This only affects if you build your own man pages in a vpath build, so it's not critical. I'll look at it later. -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Triggers on columns
On Sun, 2009-10-04 at 22:07 -0400, Tom Lane wrote: In short: while I haven't looked at the patch, I think Peter may be steering you in the wrong direction. In cases where you do have related functions, I suggest having SQL-callable V1 functions that absorb their arguments in this style, and then have them call a common subroutine that's a plain C function. Yeah, that's what he did. So forget what I said. :-) -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Making hash indexes worthwhile
On Mon, Oct 5, 2009 at 6:49 AM, Tom Lane t...@sss.pgh.pa.us wrote: Jeff Janes jeff.ja...@gmail.com writes: Do you know why that should be? I've done some work with gprof, and the results are pretty suspect, because the total gprof time adds up to only about 1/3 of the total time the backend spends on CPU (according to top), and I don't know where the unaccounted for time is going. Are you sure that gprof is delivering trustworthy numbers at all? I've seen cases where it was consistently mis-timing things. https://bugzilla.redhat.com/show_bug.cgi?id=151763 Admittedly that was an old Linux version, but ... That bug goes in the other direction versus what I am seeing, reporting more time than the clock on the wall would allow. I think things are more or less good, because profiling simple programs gives the expected answers, where under that bug they wouldn't. I think there are some kinds of system calls which are accounted for as user-space by top, but which are not interruptable and so don't get timed by gprof. But are such events evenly distributed throughout the code or concentrated in certain places? I'll need to experiment with it a bit more. Any case, I hacked the hash index code to not take heavy locks on meta-block or on bucket-blocks (lw bufffer content locks are still taken) and the performance in a nested loop (full table scan outer, hash-index inner scan) went up by over 50%, with only one backend active. And the heavy-weight locking code dropped out of the top spots in gprof. So I think I may be on to something. I want to run it for many more hours to make sure I'm getting good statistics. Then it is a question of whether my other ideas for making it safe to remove the heavy weight locks on a non-read-only system can be implemented. Jeff -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] Largeobject access controls
Tom Lane wrote: KaiGai Kohei kai...@ak.jp.nec.com writes: I rebased the largeobject access controls patch to the CVS HEAD because of the patch confliction to the default ACL patch. Quick comment on this --- I think that using a syscache for large objects is probably not a good idea. There is no provision in the catcache code for limiting the cache size anymore, and that means that anybody who touches a large number of large objects is going to blow out memory. We removed the old cache limit code because that seemed most sensible for the use of the caches for regular catalog objects, but I don't think LOs will have the same characteristics with respect to either number of objects or locality of access. The attached patch is the revised largeobject access controls. It replaced any usage of system cache for pg_largeobject_metadata. In this patch, we basically follow the manner in the pg_tablespace which also does not have its own system cache. For example, it needs to open the pg_largeobject_metadata relation with AccessShareLock when pg_largeobject_aclcheck() is called, as pg_tablespace_aclcheck() doing. $ diffstat sepgsql-02-blob-8.5devel-r2354.patch.gz doc/src/sgml/config.sgml | 28 + doc/src/sgml/ref/allfiles.sgml|1 doc/src/sgml/ref/alter_large_object.sgml | 75 doc/src/sgml/ref/grant.sgml |8 doc/src/sgml/ref/revoke.sgml |6 doc/src/sgml/reference.sgml |1 src/backend/catalog/Makefile |6 src/backend/catalog/aclchk.c | 294 ++ src/backend/catalog/dependency.c | 14 src/backend/catalog/pg_largeobject.c | 406 + src/backend/catalog/pg_shdepend.c |5 src/backend/commands/alter.c |5 src/backend/commands/comment.c|7 src/backend/commands/tablecmds.c |1 src/backend/libpq/be-fsstubs.c| 49 +- src/backend/parser/gram.y | 20 + src/backend/storage/large_object/inv_api.c| 108 +---! src/backend/tcop/utility.c|3 src/backend/utils/adt/acl.c |5 src/backend/utils/misc/guc.c | 10 src/backend/utils/misc/postgresql.conf.sample |1 src/bin/psql/large_obj.c | 10 src/include/catalog/dependency.h |1 src/include/catalog/indexing.h|3 src/include/catalog/pg_largeobject.h |4 src/include/catalog/pg_largeobject_metadata.h | 68 src/include/nodes/parsenodes.h|1 src/include/utils/acl.h |6 src/test/regress/expected/privileges.out | 206 + src/test/regress/expected/sanity_check.out|3 src/test/regress/sql/privileges.sql | 84 + 31 files changed, 1166 insertions(+), 80 deletions(-), 193 modifications(!) -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com sepgsql-02-blob-8.5devel-r2354.patch.gz Description: application/gzip -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] Streaming Replication patch for CommitFest 2009-09
Hi, On Tue, Oct 6, 2009 at 10:42 PM, Alvaro Herrera alvhe...@commandprompt.com wrote: Hmm. Without looking at the patch at all, this seems similar to how autovacuum does things: autovac launcher signals postmaster that a worker needs to be started. Postmaster proceeds to fork a worker. This could obviously fail for a lot of reasons. Yeah, I drew upon the autovac code. Now, there is code in place to notify the user when forking fails, and this is seen on the wild quite a bit more than one would like :-( I think it would be a good idea to have a retry mechanism in the walreceiver startup mechanism so that recovery does not get stuck due to transient problems. Agreed. The latest patch provides the retry mechanism. Regards, -- Fujii Masao NIPPON TELEGRAPH AND TELEPHONE CORPORATION NTT Open Source Software Center -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] [PATCH] DefaultACLs
Tom Lane wrote: Petr Jelinek pjmo...@pjmodos.net writes: KaiGai Kohei napsal(a): SELECT INTO does not check ACL_INSERT on the newly created tables, because we had been able to assume the table owner always has privilege to insert values into the new table. So, OpenIntoRel() didn't check this obvious privilege. But the default ACL feature breaks this assumption. The table owner may not have privilege to insert values into new tables. So, it is necessary to put actual access controls on the OpenIntoRel(). That's strange behavior I agree. However I don't see how default ACLs changed it in any way, owner could REVOKE his privileges before. The point is that some rows got into the new table, which should have been disallowed if CREATE TABLE AS is considered to represent a CREATE followed by an INSERT. However, I disagree that this necessarily represents a bug in the permissions checks. We could perfectly well define that INSERT means the right to insert into the table *after it is created*, and that CREATE TABLE AS does not involve this right. I think that that is a reasonable and potentially useful thing to do, whereas defining it as KaiGai-san suggests would have no usefulness whatsoever. What's more, CREATE TABLE AS is specified in the SQL standard, and I see nothing in the standard mentioning that it requires INSERT privilege. It is also an issue due to the differences in perspectives and security models. My major concern is come from the viewpoint of MAC which tries to control data flows based on sensitivity levels. So, if the default PG model considers that CREATE TABLE AS is an atomic operation, not a pair of CREATE and INSERTs, I think it is an approach to understand this behavior. In my preference, this perspective should be documented somewhere. (source code comments or official documentation?) BTW, in the MAC model, we must strictly prevent a user with classified security level to write any data into objects with unclassified security level. It is called write-down, being equivalent to information leaks. Thanks, -- OSS Platform Development Division, NEC KaiGai Kohei kai...@ak.jp.nec.com -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] COPY enhancements
I just realized that I forgot to CC the list when I answered to Josh... resending! Josh, I think that this was the original idea but we should probably rollback the error logging if the command has been rolled back. It might be more consistent to use the same hi_options as the copy command. Any idea what would be best? Well, if we're logging to a file, you wouldn't be *able* to roll them back. Also, presumbly, if you abort a COPY because of errors, you probably want to keep the errors around for later analysis. No? You are right about the file (though this functionality is not implemented yet). I don't have any strong opinion about the right behavior if COPY aborts. The error log table would contain tuples that were not inserted anyway. Now knowing whether the bad tuples come from a successful COPY command or not will be left to the user. Emmanuel -- Emmanuel Cecchet Aster Data Systems Web: http://www.asterdata.com
Re: [HACKERS] Encoding issues in console and eventlog on win32
Magnus Hagander mag...@hagander.net wrote:
First of all, the change to port/open.c seems to be unrelated to the
rest, and should be a separate patch, correct? I'm sure there's a
usecase for it, but it's not actually included in the patches
description, so I assume this was a mistake?
It was just a demo for pgwin32_toUTF16(). I'll remove this part from
the patch, but I think we also need to fix the encoding mismatch issue
in path strings. I'll re-submit for the next commitfest.
Per your own comments earlier, and in the code, what will happen if
pg_do_encoding_conversion() calls ereport()? Didn't you say we need a
non-throwing version of it?
We are hard to use encoding conversion functions in logging routines
because they could throw errors if there are some unconvertable characters.
Non-throwing version will convert such characters into '?' or escaped form
(something like \888 or \xFF). If there where such infrastructure, we can
support log_encoding settings and convert messages in platform-dependent
encoding before writing to syslog or console.
pgwin32_toUTF16() needs error checking on the API calls, and needs to
do something reasonable if it fails.
Now it returns NULL and caller writes messages in the original encoding.
Also I added the following error checks before calling pgwin32_toUTF16()
(errordata_stack_depth ERRORDATA_STACK_SIZE - 1)
to avoid recursive errors, but I'm not sure it is really meaningful.
Please remove or rewrite this part if it is not a right way.
The encoding_to_codepage array needs to go in encnames.c, where other
such tables are. Perhaps it can even be integrated in pg_enc2name_tbl
as a separate field?
I added pg_enc2name.codepage. Note that this field is needed only
on Windows, but now exported for all platforms. If you don't like
the useless field, the following macro could be a help.
#ifdef WIN32
#define def_enc2name(name, codepage){ #name, PG_##name, codepage }
#else
#define def_enc2name(name, codepage){ #name, PG_##name }
#endif
pg_enc2name pg_enc2name_tbl[] =
{
def_enc2name(SQL_ASCII),
def_enc2name(EUC_JP),
...
I don't have the time to clean this up right now, so if you have,
please do so and resubmit. If not, I can clean it up later and apply.
Patch attached.
Regards,
---
ITAGAKI Takahiro
NTT Open Source Software Center
eventlog_20091007.patch
Description: Binary data
--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers
