Re: [PATCHES] Tablespaces
On Sun, 20 Jun 2004, Simon Riggs wrote: On Thu, 2004-05-27 at 07:59, Gavin Sherry wrote: Attached is my latest patch implementing tablespaces. This has all the functionality I was planning for 7.5. Most of the information about the patch is contained in the patch/documentation, previous submissions and the archives. Testing, review, comments would be greatly appreciated. I've reviewed your patch by eye, but can't see anything in your patch about relocating the pg_xlog directory. I didn't intend on looking at that in this patch. pg_xlog is only referred to in 4 lines in the code (incl. PITR patch): - xlog.c - pgarch.c (PITR patch) - initdb.c - pgresetxlog.c Each time it is simply setting a string to the location of the xlog directory. If we could work out a way of... i) letting the pg_xlog be created by default ii) then transferring this to another tablespace later? That would give us maximum flexibility, since you may wish to change location later when workload changes/increases. Sounds reasonable. Perhaps adding a GUC...for wal_tablespace (pls suggest name!) defaults to the pg_xlog directory, when not listed? Changeable only at postmaster startup... This could be done independently of tablespaces, but I think any directory flexibility/change should work using the tablespace infrastructure, not in addition to it. If the change is as simple as you propose, it has nothing to do with the tablespace code. Also, I don't see any situation where we would want to make use of the tablespace code. Could we discuss this? it sounds like a change we could make happen fairly quickly now your code is in place. Again, I don't think my code really has any affect on the location of xlog. Of course, I accept that many may say that such a change is not really needed, but then... Comments anyone? Gavin ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
Re: [PATCHES] Tablespaces
I'm starting to review this patch, and almost immediately came across
what seemed a spectacularly bad idea:
*** src/backend/storage/buffer/bufmgr.c8 May 2004 19:09:25 -1.165
--- src/backend/storage/buffer/bufmgr.c26 May 2004 06:21:01 -
***
*** 1148,1152
{
bufHdr = LocalBufferDescriptors[i];
! if (RelFileNodeEquals(bufHdr-tag.rnode, rnode))
{
bufHdr-flags = ~(BM_DIRTY | BM_JUST_DIRTIED);
--- 1148,1156
{
bufHdr = LocalBufferDescriptors[i];
! /* special case for default tblNode */
! if (RelFileNodeEquals(bufHdr-tag.rnode, rnode) ||
! (!OidIsValid(rnode.tblNode)
! bufHdr-tag.rnode.relNode == rnode.relNode
! bufHdr-tag.rnode.dbNode == rnode.dbNode))
{
bufHdr-flags = ~(BM_DIRTY | BM_JUST_DIRTIED);
There has got to be a better way than this. In the first place the
code seems able to seize on the wrong buffer if it's not checking
all three fields; in the second place, if the weak matching is correct
here why is it not needed everyplace else?
What's the purpose of this?
regards, tom lane
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Tablespaces
Hi Tom,
On Wed, 16 Jun 2004, Tom Lane wrote:
I'm starting to review this patch, and almost immediately came across
what seemed a spectacularly bad idea:
*** src/backend/storage/buffer/bufmgr.c8 May 2004 19:09:25 -1.165
--- src/backend/storage/buffer/bufmgr.c26 May 2004 06:21:01 -
***
*** 1148,1152
{
bufHdr = LocalBufferDescriptors[i];
! if (RelFileNodeEquals(bufHdr-tag.rnode, rnode))
{
bufHdr-flags = ~(BM_DIRTY | BM_JUST_DIRTIED);
--- 1148,1156
{
bufHdr = LocalBufferDescriptors[i];
! /* special case for default tblNode */
! if (RelFileNodeEquals(bufHdr-tag.rnode, rnode) ||
! (!OidIsValid(rnode.tblNode)
! bufHdr-tag.rnode.relNode == rnode.relNode
! bufHdr-tag.rnode.dbNode == rnode.dbNode))
{
bufHdr-flags = ~(BM_DIRTY | BM_JUST_DIRTIED);
There has got to be a better way than this. In the first place the
code seems able to seize on the wrong buffer if it's not checking
all three fields; in the second place, if the weak matching is correct
here why is it not needed everyplace else?
Ahh. This is a hang over from some tests I was doing. I must have missed
it when I send the patch in. The patch should certainly work without this
change. I will verify later today when I have access to my development
machine.
Gavin
---(end of broadcast)---
TIP 3: if posting/reading through Usenet, please send an appropriate
subscribe-nomail command to [EMAIL PROTECTED] so that your
message can get through to the mailing list cleanly
Re: [PATCHES] Tablespaces
Your patch has been added to the PostgreSQL unapplied patches list at: http://momjian.postgresql.org/cgi-bin/pgpatches I will try to apply it after review.. --- Gavin Sherry wrote: Attached is an updated patch, fixing a compile error which my compiler didn't seem to detect/suffer from and incorporating fixes to problems raised by Neil. Thanks, Gavin Content-Description: [ Attachment, skipping... ] ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED]
Re: [PATCHES] Tablespaces
Yes, I was wondering if it was ready for application. Tom, you want to eyeball it? --- Christopher Kings-Lynne wrote: Just reminding someone to review this some time...it does all seem to work very well :) Chris Gavin Sherry wrote: Attached is an updated patch, fixing a compile error which my compiler didn't seem to detect/suffer from and incorporating fixes to problems raised by Neil. Thanks, Gavin ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match ---(end of broadcast)--- TIP 1: subscribe and unsubscribe commands go to [EMAIL PROTECTED] -- Bruce Momjian| http://candle.pha.pa.us [EMAIL PROTECTED] | (610) 359-1001 + If your life is a hard drive, | 13 Roberts Road + Christ can be your backup.| Newtown Square, Pennsylvania 19073 ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
[PATCHES] Tablespaces
Hi all, Attached is my latest patch implementing tablespaces. This has all the functionality I was planning for 7.5. Most of the information about the patch is contained in the patch/documentation, previous submissions and the archives. Testing, review, comments would be greatly appreciated. Gavin tablespace-18.diff.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 7: don't forget to increase your free space map settings
Re: [PATCHES] Tablespaces
Gavin Sherry wrote:
Attached is my latest patch implementing tablespaces. This has all the
functionality I was planning for 7.5.
A few minor points I happened to notice while reading through the patch:
+ * To simply initialisation and XLog activity, have create and
maintain
+ * a symbolic link map in data/pg_tablespaces.
Grammar errors.
+ void
+ TblspcCreateDbspace(Oid tbloid)
+ {
+ #ifndef HAVE_SYMLINK
+ return;
+ #endif
+ struct stat st;
+ char*dir;
If HAVE_SYMLINK is undefined, this is a syntax error (at least in
C89, which is what we ought to limit ourselves to). Similar problems
elsewhere in the same file (tablespc.c)
+ dir = (char *) palloc(strlen(DataDir) + 14 + 10 + 10 + 10 + 3 + 1);
+ sprintf(dir, %s/pg_tablespaces/%u/%u, DataDir, tbloid,
+ MyDatabaseId);
Is the length of that buffer right? At the least the addition is a
little weird (why are you adding 10 three times for two numeric
variables?) I noticed another buffer allocation (linkloc) that
looked dubious at first glance as well.
+ charrealnewpath[MAXPGPATH];
This is somewhat pedantic, but how do we know that MAXPGPATH =
PATH_MAX (the minimum safe size of the second argument to
realpath(), at least on my local system)?
-Neil
---(end of broadcast)---
TIP 4: Don't 'kill -9' the postmaster
Re: [PATCHES] Tablespaces
Attached is an updated patch, fixing a compile error which my compiler didn't seem to detect/suffer from and incorporating fixes to problems raised by Neil. Thanks, Gavin tablespace-20.diff.gz Description: GNU Zip compressed data ---(end of broadcast)--- TIP 9: the planner will ignore your desire to choose an index scan if your joining column's datatypes do not match
