Why SyncRepWakeQueue is not static?

Started by Tatsuo Ishiialmost 11 years ago6 messages
#1Tatsuo Ishii
ishii@postgresql.org
1 attachment(s)

SyncRepWakeQueue (src/backend/replication/syncrep.c) is not used
anywhere except in the file. If there's no good reason for it, I think
it should be declared as a static function. Included patch does so.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

Attachments:

syncrep.patchtext/x-patch; charset=us-asciiDownload
diff --git a/src/backend/replication/syncrep.c b/src/backend/replication/syncrep.c
index ec594cf..325239d 100644
--- a/src/backend/replication/syncrep.c
+++ b/src/backend/replication/syncrep.c
@@ -69,6 +69,7 @@ static int	SyncRepWaitMode = SYNC_REP_NO_WAIT;
 
 static void SyncRepQueueInsert(int mode);
 static void SyncRepCancelWait(void);
+static int	SyncRepWakeQueue(bool all, int mode);
 
 static int	SyncRepGetStandbyPriority(void);
 
@@ -546,7 +547,7 @@ SyncRepGetStandbyPriority(void)
  *
  * Must hold SyncRepLock.
  */
-int
+static int
 SyncRepWakeQueue(bool all, int mode)
 {
 	volatile WalSndCtlData *walsndctl = WalSndCtl;
diff --git a/src/include/replication/syncrep.h b/src/include/replication/syncrep.h
index b3d399d..71e2857 100644
--- a/src/include/replication/syncrep.h
+++ b/src/include/replication/syncrep.h
@@ -47,9 +47,6 @@ extern void SyncRepReleaseWaiters(void);
 /* called by checkpointer */
 extern void SyncRepUpdateSyncStandbysDefined(void);
 
-/* called by various procs */
-extern int	SyncRepWakeQueue(bool all, int mode);
-
 /* forward declaration to avoid pulling in walsender_private.h */
 struct WalSnd;
 extern struct WalSnd *SyncRepGetSynchronousStandby(void);
#2Michael Paquier
michael.paquier@gmail.com
In reply to: Tatsuo Ishii (#1)
Re: Why SyncRepWakeQueue is not static?

On Wed, Mar 25, 2015 at 12:13 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:

SyncRepWakeQueue (src/backend/replication/syncrep.c) is not used
anywhere except in the file. If there's no good reason for it, I think
it should be declared as a static function. Included patch does so.

That's indeed contradictory with what is written in syncrep.h, the
function is not called from other places.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#3Tatsuo Ishii
ishii@postgresql.org
In reply to: Tatsuo Ishii (#1)
Re: Why SyncRepWakeQueue is not static?

SyncRepWakeQueue (src/backend/replication/syncrep.c) is not used
anywhere except in the file. If there's no good reason for it, I think
it should be declared as a static function. Included patch does so.

Fix committed/pushed from master to 9.2. 9.1 declares it as a static
function.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#4Michael Paquier
michael.paquier@gmail.com
In reply to: Tatsuo Ishii (#3)
Re: Why SyncRepWakeQueue is not static?

On Thu, Mar 26, 2015 at 10:46 AM, Tatsuo Ishii <ishii@postgresql.org> wrote:

SyncRepWakeQueue (src/backend/replication/syncrep.c) is not used
anywhere except in the file. If there's no good reason for it, I think
it should be declared as a static function. Included patch does so.

Fix committed/pushed from master to 9.2. 9.1 declares it as a static
function.

Er, is that a good idea to back-patch that? Normally routine specs are
maintained stable on back-branches, and this is just a cosmetic
change.
--
Michael

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#5Tatsuo Ishii
ishii@postgresql.org
In reply to: Michael Paquier (#4)
Re: Why SyncRepWakeQueue is not static?

Fix committed/pushed from master to 9.2. 9.1 declares it as a static
function.

Er, is that a good idea to back-patch that? Normally routine specs are
maintained stable on back-branches, and this is just a cosmetic
change.

I'm not sure if it's a cosmetic change or not. I thought declaring
to-be-static function as extern is against our coding
standard. Moreover, if someone wants to change near the place in the
source code in the future, changes made to head may not be easily back
patched or cherry-picked to older branches if I do not back patch it.

Best regards,
--
Tatsuo Ishii
SRA OSS, Inc. Japan
English: http://www.sraoss.co.jp/index_en.php
Japanese:http://www.sraoss.co.jp

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

#6Robert Haas
robertmhaas@gmail.com
In reply to: Tatsuo Ishii (#5)
Re: Why SyncRepWakeQueue is not static?

On Wed, Mar 25, 2015 at 10:07 PM, Tatsuo Ishii <ishii@postgresql.org> wrote:

Fix committed/pushed from master to 9.2. 9.1 declares it as a static
function.

Er, is that a good idea to back-patch that? Normally routine specs are
maintained stable on back-branches, and this is just a cosmetic
change.

I'm not sure if it's a cosmetic change or not. I thought declaring
to-be-static function as extern is against our coding
standard. Moreover, if someone wants to change near the place in the
source code in the future, changes made to head may not be easily back
patched or cherry-picked to older branches if I do not back patch it.

True. But if any third-party code calls that function, you just broke
it. I don't think keeping the back-branches consistent with master is
a sufficiently good reason for such a change.

--
Robert Haas
EnterpriseDB: http://www.enterprisedb.com
The Enterprise PostgreSQL Company

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers